Skip to content

Conversation

@lslusarczyk
Copy link
Contributor

Change summary

checkUrResult function was made as short as possible by moving away all error handling to a separate non-inlined function and keeping common success path tiny and additionally optimized by compiler __builtin_expect. Also a few more simple inlines were added.

Performance impact

checkUrResult function is widely used in all paths including common path. My optimization positive impact on performance is visible in almost all metrics.

Some examples are below:

SYCL instructions reduced from 153.9k to 151.8k over UR (130.9), that is overhead over UR reduced by 9.1%, see:
SubmitKernel out of order with completion using events, CPU count(1)

SYCL instructions reduced from 133.5k to 132.2k over UR (119.2), that is overhead over UR reduced by 9.1%, see:
SubmitKernel in order, CPU count(2)

SYCL instructions reduced from 145.9k to 143.9k over UR (118.4), that is overhead over UR reduced by 7.3%, see:
SubmitKernel out of order long kernel, CPU count(3)

SYCL time reduced from 202.1 to 194.6 over UR (155.0), that is overhead over UR reduced by 15.9%, see:
SinKernelGraph, numKernels_ 100(1)

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM! Just some small suggestions.

Copy link
Contributor

@ldorau ldorau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UR changes LGTM

@lslusarczyk
Copy link
Contributor Author

@intel/llvm-gatekeepers , please merge
Gaps in this PR testing are not related to my change

@steffenlarsen
Copy link
Contributor

Looks like the UR testing is still running. Are they thought to be stuck or can we wait for them to finish?

@lslusarczyk
Copy link
Contributor Author

Looks like the UR testing is still running. Are they thought to be stuck or can we wait for them to finish?

I don't see it running now. It was running in the morning and I was told by devops it is stuck. There is end of a day so I think it is not worth to wait more for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants