-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Change NativePrograms.insert to []
access
#14873
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
[]
access[]
access
1b87165
to
25f8ef8
Compare
Strictly speaking, this is a functional change without a test. I understand that there is probably an out-of-tree test that exposes this, but I also assume that it should be possible to write a unit-test for this so we catch that earlier if someone breaks this again |
25f8ef8
to
b3dcb71
Compare
`map.insert` doesn't insert values if the set already contains them. This can happen when UR/PI happens to reuse the same program pointer that it used for a previous program.
b3dcb71
to
7e09c61
Compare
Test added. I also removed the comments - hopefully now that there's a proper test, there's no need to explicitly write out the footgun every time. |
Merging now to attempt to unbreak CI, CUDA passed on the self-hosted runner and I highly doubt any HIP-specific issue, the runner is totally slammed. |
@RossBrunton Seeing some postcommit failures (although build passes now), can you please investigate?
|
Hey @sarnex , I have took a fast look on that and reverted the problematic part here. I could confirm this should fix these failures, but couldn't confirm that it won't introduce the |
Thanks, will take a look |
@omarahmed1111 @RossBrunton hi, are you going to re-introduce this patch? |
I've just realized that this PR could be an incorrect change. 4bf1fe3 made I think that we should re-instate that multimap first and then review its uses to see if any changes are needed. |
Thanks for looking into this, everyone. I've tested the map->multimap test, and it seems to fix things locally for me. So I'll make an MR reverting this and fixing the map issue. |
4f86ab replaced `insert` with `[]` in order to fix a pointer re-use issue, however that was not the correct fix. Instead, a multimap was incorrectly converted to a regular map during the PI->UR conversion. This change reverts the rest of 4f86ab (besides the test, which is still valid), and converts NativePrograms back to a multimap. --- Thanks to @AlexeySachkov for finding the real issue in #14873 (comment)
4f86ab replaced `insert` with `[]` in order to fix a pointer re-use issue, however that was not the correct fix. Instead, a multimap was incorrectly converted to a regular map during the PI->UR conversion. This change reverts the rest of 4f86ab (besides the test, which is still valid), and converts NativePrograms back to a multimap. --- Thanks to @AlexeySachkov for finding the real issue in intel#14873 (comment)
map.insert
doesn't insert values if the set already contains them. This can happen when UR/PI happens to reuse the same program pointer that it used for a previous program.--
This was causing some tests in the PI 2 UR conversion to randomly fail, including at least #14765 .
Fixes #14819.