Skip to content

[SYCL] Cache kernel assert usage #18538

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: sycl
Choose a base branch
from

Conversation

sergey-semenov
Copy link
Contributor

Cache whether a kernel uses assertions or not to avoid a set lookup.

@sergey-semenov sergey-semenov requested a review from a team as a code owner May 19, 2025 13:38
@sergey-semenov sergey-semenov added the performance Performance related issues label May 19, 2025
@sergey-semenov sergey-semenov requested a review from againull May 19, 2025 13:38
Comment on lines +365 to +366
if (!KernelNameBasedCachePtr)
return m_KernelUsesAssert.find(KernelName) != m_KernelUsesAssert.end();
Copy link
Contributor

Choose a reason for hiding this comment

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

What conditions could lead to us having a kernel name but no cache associated with it?

Copy link
Contributor Author

@sergey-semenov sergey-semenov May 19, 2025

Choose a reason for hiding this comment

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

An application that hasn't been recompiled since the cache was added to the headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, wow, okay.

Would it make sense to mark this as #ifndef __INTEL_PREVIEW_BREAKING_CHANGES? My thinking is:

  • An application must recompile to use the preview library, and will need to recompile after the ABI break;
  • In the case where somebody has recompiled, we can skip a branch; and
  • Decorating this with the #ifdef will make sure it gets taken out at the next ABI break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I think it makes sense. Since this is also applicable to the UR kernel cache logic introduced by the original cache PR, I'd rather apply this to both in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants