Skip to content

[DeviceMSAN] Fix false negative report due to __spirv_GroupAsyncCopy #18216

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

Merged
merged 14 commits into from
May 21, 2025

Conversation

AllanZyne
Copy link
Contributor

@AllanZyne AllanZyne commented Apr 28, 2025

"__spirv_GroupAsyncCopy" is used to copy data between local and global buffer, we need to sync the shadow value of them.
I add a new function "__msan_unpoison_strided_copy" for this sync.

@AllanZyne AllanZyne marked this pull request as ready for review May 16, 2025 07:01
@AllanZyne AllanZyne requested a review from a team as a code owner May 16, 2025 07:01
Copy link
Contributor

@yingcong-wu yingcong-wu left a comment

Choose a reason for hiding this comment

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

Please update the PR's description when creating one. Like giving the context of this change and some high level summary.

@yingcong-wu yingcong-wu requested a review from Copilot May 16, 2025 07:16
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends DeviceMSAN to correctly handle the SPIR-V __spirv_GroupAsyncCopy intrinsic by adding a strided copy callback, demangling helper, and RTL implementation for unpoisoning shadows.

  • Registers a new __msan_unpoison_strided_copy callback in the SPIR pass
  • Skips sanitization for __spirv_GroupAsyncCopy calls and emits a call to the new unpoison function
  • Implements the strided-copy logic and debug messaging in the msan_rtl.cpp

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File Description
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp Added callback registration, demangling helper, and visitor hook
libdevice/sanitizer/msan_rtl.cpp Introduced __msan_unpoison_strided_copy RTL with template copy
Files not reviewed (1)
  • llvm/test/Instrumentation/MemorySanitizer/SPIRV/spirv_groupasynccopy.ll: Language not supported
Comments suppressed due to low confidence (1)

libdevice/sanitizer/msan_rtl.cpp:604

  • Add unit or integration tests to verify __msan_unpoison_strided_copy correctly handles various element sizes and stride values to prevent regressions.
void __msan_unpoison_strided_copy(uptr dest, uint32_t dest_as, uptr src,

@AllanZyne AllanZyne requested a review from Copilot May 19, 2025 07:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes a false negative report caused by misidentifying __spirv_GroupAsyncCopy. Key changes include adding a new callback function (__msan_unpoison_strided_copy) with its registration in the sanitizer, introducing helper logic to demangle __spirv_GroupAsyncCopy names, and extending RTL support via a templated GroupAsyncCopy.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File Description
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp Added friend declaration, new FunctionCallee for MsanUnpoisonStridedCopy, helper function for mangling demangle, and adjusted metadata exclusion for __spirv_GroupAsyncCopy calls
libdevice/sanitizer/msan_rtl.cpp Introduced a templated GroupAsyncCopy helper and implemented __msan_unpoison_strided_copy to support strided copy unpoisoning
Files not reviewed (1)
  • llvm/test/Instrumentation/MemorySanitizer/SPIRV/spirv_groupasynccopy.ll: Language not supported

@AllanZyne AllanZyne requested a review from Copilot May 19, 2025 07:57
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Fix false negative MSan reports for __spirv_GroupAsyncCopy by adding a custom strided-copy unpoison callback and associated runtime support.

  • Register and call a new __msan_unpoison_strided_copy in the SPIR-V instrumenter
  • Exclude __spirv_GroupAsyncCopy from the no-sanitize filter and demangle its element type
  • Implement device-side shadow propagation for strided copies in the MSan RTL

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File Description
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp Added MsanUnpoisonStridedCopyFunc, excluded GroupAsyncCopy from no-sanitize, demangled vector types, and emitted the strided-copy call
libdevice/sanitizer/msan_rtl.cpp Defined __msan_unpoison_strided_copy, shadow-copy loop template, and error message for unsupported types
Files not reviewed (1)
  • llvm/test/Instrumentation/MemorySanitizer/SPIRV/spirv_groupasynccopy.ll: Language not supported
Comments suppressed due to low confidence (1)

libdevice/sanitizer/msan_rtl.cpp:602

  • The identifier __msan_print_strided_copy_unsupport_type has a typo; rename to __msan_print_strided_copy_unsupported_type for consistency.
static __SYCL_CONSTANT__ const char __msan_print_strided_copy_unsupport_type[] = "[kernel] __msan_unpoison_strided_copy: unsupported type(%d)\n";

@yingcong-wu
Copy link
Contributor

Please update the PR's description when creating one. Like giving the context of this change and some high level summary.

Hi @AllanZyne , please update the description, thanks.

@yingcong-wu yingcong-wu requested a review from zhaomaosu May 20, 2025 03:05
@AllanZyne
Copy link
Contributor Author

Hi @intel/llvm-gatekeepers, the failed tests are infrastructure issues, this PR is ready to merge, thanks!

@aelovikov-intel aelovikov-intel merged commit df4bd05 into sycl May 21, 2025
41 of 47 checks passed
@aelovikov-intel aelovikov-intel deleted the review/yang/fix_groupasynccopy branch May 21, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants