-
Notifications
You must be signed in to change notification settings - Fork 769
[DeviceSanitizer][NFC] Add comments for initialization of LaunchInfo.Data. #18915
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
base: sycl
Are you sure you want to change the base?
[DeviceSanitizer][NFC] Add comments for initialization of LaunchInfo.Data. #18915
Conversation
AsanInterceptor::prepareLaunch has this call, which aligns with msan and tsan.
Found the issue when investigate test failure in opencl cpu runtime. |
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.
LGTM, thanks. There is another sync op inside preLaunchKernel
, so this one is indeed duplicated and useless.
@intel/llvm-gatekeepers, pr is ready to merge. Thank you! |
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.
This call is necessary, don't remove it!
It's better to add comments if it's necessary. |
When the kernel is filtered, it's still possible that LaunchInfo is used in kernel code, so the "syncToDevice" here will give it the default value. To make this PR meaningful, could you help to add a comment to tip others not remove this line? |
I agree. If this is necessary, then the code should be commented for its necessity to avoid such problem. |
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.
Add comment from @AllanZyne and do not remove this line.
…dd comments. This reverts commit 10ab6c8.
AsanInterceptor::prepareLaunch has this call, which aligns with msan and tsan.