-
Notifications
You must be signed in to change notification settings - Fork 802
[SYCL] Refactor kernel name based data approach #19117
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
Conversation
Additional cleanups enabled by intel#19117.
Additional cleanups enabled by intel#19117.
| if (isCompileTimeInfoSet()) | ||
| CompileTimeKernelInfoTy::operator=(Info); | ||
| assert(isCompileTimeInfoSet()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergey-semenov , what did you mean by those lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, this should be if(!isCompileTimeInfoSet())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why isn't it failing anywhere?
- When I tried the change (although on top of my other changes), things broke for me. I'm not sure if it'd be clean on trunk either. Will you follow up please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't it failing anywhere?
That probably indicates lack of test coverage for this scenario. If that's true, I don't think it's worth adding since my next PR will essentially make all kernel info behave like this (i.e. runtime information will be filled out during image registration, compile time information will be filled out during the first access to device kernel info with compile time information available).
When I tried the change (although on top of my other changes), things broke for me. I'm not sure if it'd be clean on trunk either. Will you follow up please?
I'll look into it, let's see if #20003 hits any failures in precommit.
KernelNameBasedCacheTtoDeviceKernelInfoas that reflects its usage better and avoids the confusion with kernel caches.