Skip to content

[DeviceSanitizer] Move isUnsupportedSPIRAccess to SPIRVSanitizerCommonUtils.cpp. #17597

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 6 commits into from
Apr 23, 2025
Merged

[DeviceSanitizer] Move isUnsupportedSPIRAccess to SPIRVSanitizerCommonUtils.cpp. #17597

merged 6 commits into from
Apr 23, 2025

Conversation

haonanya1
Copy link
Contributor

Both AddressSanitizer and MemorySanitizer ignore target extension type.

@haonanya1 haonanya1 requested review from a team as code owners March 24, 2025 05:34
yingcong-wu
yingcong-wu reviewed Mar 24, 2025
View reviewed changes
llvm/test/Instrumentation/MemorySanitizer/SPIRV/ignore_target_ext_type.ll
Copy link
Contributor

Choose a reason for hiding this comment

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

There is sanitize_address and ; CHECK-NOT: call void @asan_load in this file. Is something wrong here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't check lit, let me double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks.

@haonanya1 haonanya1 marked this pull request as draft March 24, 2025 05:45
@haonanya1 haonanya1 marked this pull request as ready for review March 24, 2025 06:45
@haonanya1 haonanya1 marked this pull request as draft March 24, 2025 06:46
@AllanZyne
Copy link
Contributor

AllanZyne commented Mar 25, 2025 •
edited
Loading

IMO, the file name "SanitizerCommonUtils" is too general, it doesn't look like a file specific to the device sanitizer.
Maybe, "SpirvSanitizerCommonUtils" is better.

@haonanya1
Copy link
Contributor Author

IMO, the file name "SanitizerCommonUtils" is too general, it doesn't look like a file specific to the device sanitizer. Maybe, "SpirvSanitizerCommonUtils" is better.

There is a file named SPIRITTAnnotations.h, What about SPIRVSanitizerCommonUtils?

@haonanya1 haonanya1 changed the title [DeviceSanitizer] Move isUnsupportedSPIRAccess to SanitizerCommonUtils namespace. [DeviceSanitizer] Move isUnsupportedSPIRAccess to SPIRVSanitizerCommonUtils namespace. Mar 25, 2025
@haonanya1 haonanya1 marked this pull request as ready for review March 25, 2025 05:53
@haonanya1 haonanya1 requested a review from yingcong-wu March 25, 2025 05:53
@haonanya1
Copy link
Contributor Author

@intel/dpcpp-tools-reviewers, could you please help me to trigger CI? Thanks.

@haonanya1 haonanya1 mentioned this pull request Mar 26, 2025
Draft
@haonanya1 haonanya1 changed the title [DeviceSanitizer] Move isUnsupportedSPIRAccess to SPIRVSanitizerCommonUtils namespace. [DeviceSanitizer] Move isUnsupportedSPIRAccess to SPIRVSanitizerCommonUtils.cpp. Mar 26, 2025
@haonanya1 haonanya1 requested a review from a team as a code owner April 17, 2025 06:12
uditagarwal97
uditagarwal97 approved these changes Apr 17, 2025
View reviewed changes
Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

CODEOWNERS file LGTM

zhaomaosu
zhaomaosu approved these changes Apr 17, 2025
View reviewed changes
Copy link
Contributor

@zhaomaosu zhaomaosu left a comment

Choose a reason for hiding this comment

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

lgtm

@haonanya1
Copy link
Contributor Author

@intel/dpcpp-tools-reviewers , can you please take a look? Thanks.

@haonanya1
Copy link
Contributor Author

@intel/dpcpp-tools-reviewers , please help review the pr, thanks very much.

@haonanya1
Copy link
Contributor Author

@intel/llvm-gatekeepers , can you please merge the pr? Thanks very much.

@yingcong-wu yingcong-wu requested a review from a team April 23, 2025 07:13
@martygrant martygrant merged commit 8f7d08c into intel:sycl Apr 23, 2025
37 of 38 checks passed
@martygrant
Copy link
Contributor

martygrant commented Apr 23, 2025 •
edited
Loading

@haonanya1 this PR has caused a post commit failure https://github.com/intel/llvm/actions/runs/14613379957

@yingcong-wu
Copy link
Contributor

It looks like infra issue to me.

fatal: update_ref failed for ref 'HEAD': cannot lock ref 'HEAD': Unable to create '/__w/repo_cache/intel/llvm/.git/HEAD.lock': File exists.
Updating fcec14d004d8..8f7d08c645e9

@martygrant
Copy link
Contributor

that's right but there was a e2e test failing on windows but after re-trying the jobs it has passed now

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

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@uditagarwal97 uditagarwal97 uditagarwal97 approved these changes

@zhaomaosu zhaomaosu zhaomaosu approved these changes

@yingcong-wu yingcong-wu Awaiting requested review from yingcong-wu

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

Successfully merging this pull request may close these issues.
7 participants
@haonanya1 @AllanZyne @martygrant @yingcong-wu @AlexeySachkov @uditagarwal97 @zhaomaosu