Skip to content

[DeviceSanitizer] Disable ASan when the ESIMD kernel is found #14156

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jul 2, 2024
Merged

[DeviceSanitizer] Disable ASan when the ESIMD kernel is found #14156

merged 8 commits into from
Jul 2, 2024

Conversation

AllanZyne
Copy link
Contributor

@AllanZyne AllanZyne commented Jun 13, 2024 •
edited
Loading

ESIMD kernel doesn't support noinline functions, so we can't support sanitizer for it now.

We can't check if the kernel is ESIMD kernel at Unified Runtime, so we have to disable ASan completely when it found ESIMD kernel in device image.

@AllanZyne AllanZyne requested a review from a team as a code owner June 13, 2024 05:34
sarnex
sarnex reviewed Jun 13, 2024
View reviewed changes
@AllanZyne AllanZyne requested a review from sarnex June 20, 2024 03:03
@AllanZyne AllanZyne requested a review from a team as a code owner June 20, 2024 05:13
wenju-he
wenju-he reviewed Jun 20, 2024
View reviewed changes
Copy link
Contributor

@wenju-he wenju-he left a comment

Choose a reason for hiding this comment

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

add a lit test?

@AllanZyne AllanZyne changed the title [DeviceSanitizer] fix metadata in ESIMD [DeviceSanitizer] Skip ESIMD kernel when ASan is enabled Jun 25, 2024
@AllanZyne AllanZyne changed the title [DeviceSanitizer] Skip ESIMD kernel when ASan is enabled [DeviceSanitizer] Disable ASan when the ESIMD kernel is founded Jun 25, 2024
@AllanZyne AllanZyne changed the title [DeviceSanitizer] Disable ASan when the ESIMD kernel is founded [DeviceSanitizer] Disable ASan when the ESIMD kernel is found Jun 25, 2024
@AllanZyne
Copy link
Contributor Author

add a lit test?

Done

@AllanZyne AllanZyne requested a review from wenju-he June 25, 2024 05:41
wenju-he
wenju-he reviewed Jun 25, 2024
View reviewed changes
@AllanZyne AllanZyne requested a review from wenju-he June 26, 2024 08:14
wenju-he
wenju-he approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@wenju-he wenju-he left a comment

Choose a reason for hiding this comment

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

LGTM

@AllanZyne
Copy link
Contributor Author

AllanZyne commented Jun 27, 2024 •
edited
Loading

Hi @intel/llvm-gatekeepers, can you please merge this PR?
I'm sure the failed CI test is not related to this PR.
Thanks!

@steffenlarsen
Copy link
Contributor

Assert/assert_in_simultaneous_kernels.cpp failed on OpenCL CPU. It is not a failure I have seen before. Could you please make sure it is flaky and unrelated? If it is, we should open an issue for it.

@AllanZyne
Copy link
Contributor Author

Assert/assert_in_simultaneous_kernels.cpp failed on OpenCL CPU. It is not a failure I have seen before. Could you please make sure it is flaky and unrelated? If it is, we should open an issue for it.

Yes, it is flaky and unrelated.

@AllanZyne
Copy link
Contributor Author

AllanZyne commented Jul 2, 2024 •
edited
Loading

Hi @steffenlarsen, I merged this PR to latest code and that test failure has disappeared.
Maybe it has fixed by other PR.
So can you approve to merge this PR?

@steffenlarsen steffenlarsen merged commit cb71841 into sycl Jul 2, 2024
12 of 13 checks passed
@AllanZyne AllanZyne deleted the review/yang/fix_esimd branch July 2, 2024 11:38
sarnex
sarnex reviewed Jul 2, 2024
View reviewed changes
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -1411,6 +1410,21 @@ PreservedAnalyses AddressSanitizerPass::run(Module &M,
ClUseStackSafety ? &MAM.getResult<StackSafetyGlobalAnalysis>(M) : nullptr;

if (Triple(M.getTargetTriple()).isSPIR()) {
Copy link
Contributor

@sarnex sarnex Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just noticed, can you please change this check to

 if (Triple(M.getTargetTriple()).isSPIROrSPIRV()) {

We want to use the SPIR-V backend soon and that uses the SPIRV triple. Follow-up PR is fine. Thanks

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#14413

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

@sarnex sarnex sarnex approved these changes

@wenju-he wenju-he wenju-he approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AllanZyne @steffenlarsen @wenju-he @sarnex