[NVPTX][AMDGPU] Move annotation creation out of clang #14634

frasercrmck · 2024-07-18T16:44:06Z

This patch refactors the way we lower SYCL attributes and properties to NVVM annotations, through function attributes and metadata. It unifies the flow better with the SPIR-V paths at the same time.

Previously we had:

Clang handling function attributes in two places:
1. CodeGenFunction did generic lowering of attributes to function metadata
2. NVPTXTargetCodeGenInfo did its own additional lowering of attributes to NVVM annotations
Kernel properties being handled in clang. NVPTXTargetCodeGenInfo lowered kernel properties, which had already been converted to function attributes, to NVVM annotations.
Kernel properties for HIP/CUDA not being turned into function metadata. Because function metadata is how the ComputeModuleRuntimeInfo library creates runtime info, this meant that target backends couldn't act on properties, as they were lost during lowering.

Fundamentally, clang is not the correct place for handling SYCL kernel properties as it unnecessarily touches the front-end for what is really library code.

With this patch, we now have:

Clang handles function attributes in one place, lowering to function metadata
SYCLLowerIR lowers kernel properties to the same function metadata: now working for HIP and CUDA kernels too. Because function metadata is consistently present for kernel properties, they behave as expected on HIP/CUDA targets now.
A new pass converts function metadata to NVVM annotations, replacing the code we've taken out of clang.

This patch refactors the way we lower SYCL attributes and properties to NVVM annotations, through function attributes and metadata. It unifies the flow better with the SPIR-V paths at the same time. Previously we had: 1. Clang handling function attributes in two places: a. CodeGenFunction did generic lowering of attributes to function metadata b. NVPTXTargetCodeGenInfo did its own additional lowering of attributes to NVVM annotations 2. Kernel properties being handled in clang. NVPTXTargetCodeGenInfo lowered kernel properties, which had already been converted to function attributes, to NVVM annotations. 3. Kernel properties for HIP/CUDA *not* being turned into function metadata. Because function metadata is how the ComputeModuleRuntimeInfo library creates runtime info, this meant that target backends couldn't act on properties, as they were lost during lowering. Fundamentally, clang is not the correct place for handling SYCL kernel properties as it unnecessarily touches the front-end for what is really library code. With this patch, we now have: 1. Clang handles function attributes in one place, lowering to function metadata 2. SYCLLowerIR lowers kernel properties to the same function metadata: now working for HIP and CUDA kernels too. Because function metadata is consistently present for kernel properties, they behave as expected on HIP/CUDA targets now. 3. A new pass converts function metadata to NVVM annotations, replacing the code we've taken out of clang.

llvm/lib/SYCLLowerIR/TargetHelpers.cpp

sycl/test-e2e/Basic/work_group_size_prop.cpp

EwanC

Graph test changes LGTM

dm-vodopyanov

sycl/test-e2e/* LGTM

sarnex

llvm/lib/SYCLLowerIR/CMakeLists.txt lgtm

bader · 2024-07-22T22:23:21Z

@intel/dpcpp-cfe-reviewers, ping.

bader · 2024-07-29T16:46:48Z

@frasercrmck, please, resolve merge conflicts.

…tions-pass

frasercrmck · 2024-07-29T19:36:43Z

@frasercrmck, please, resolve merge conflicts.

Done, thanks. I haven't waited for all CI but Linux looks happy at least.

…tions-pass

frasercrmck · 2024-08-05T16:59:04Z

@intel/llvm-gatekeepers this looks ready to merge, thank you.

sarnex · 2024-08-05T17:59:52Z

@frasercrmck We are seeing macos post commit failures, can you please take a look?

https://github.com/intel/llvm/actions/runs/10253109905/job/28365021433

FAILED: lib/SYCLLowerIR/CMakeFiles/LLVMSYCLLowerIR.dir/SYCLCreateNVVMAnnotations.cpp.o 
ccache /Applications/Xcode_14.2.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/Users/runner/work/llvm/llvm/build/lib/SYCLLowerIR -I/Users/runner/work/llvm/llvm/src/llvm/lib/SYCLLowerIR -I/Users/runner/work/llvm/llvm/build/include -I/Users/runner/work/llvm/llvm/src/llvm/include -I/Users/runner/work/llvm/llvm/src/llvm/projects/vc-intrinsics/GenXIntrinsics/include -I/Users/runner/work/llvm/llvm/build/projects/vc-intrinsics/GenXIntrinsics/include -I/Users/runner/work/llvm/llvm/build/_deps/vc-intrinsics-src/GenXIntrinsics/include -I/Users/runner/work/llvm/llvm/build/_deps/vc-intrinsics-build/GenXIntrinsics/lib/GenXIntrinsics/../../include -I/Users/runner/work/llvm/llvm/build/_deps/vc-intrinsics-build/GenXIntrinsics/include -isystem /usr/local/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -O3 -DNDEBUG -std=c++17 -isysroot /Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -mmacosx-version-min=12.7  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT lib/SYCLLowerIR/CMakeFiles/LLVMSYCLLowerIR.dir/SYCLCreateNVVMAnnotations.cpp.o -MF lib/SYCLLowerIR/CMakeFiles/LLVMSYCLLowerIR.dir/SYCLCreateNVVMAnnotations.cpp.o.d -o lib/SYCLLowerIR/CMakeFiles/LLVMSYCLLowerIR.dir/SYCLCreateNVVMAnnotations.cpp.o -c /Users/runner/work/llvm/llvm/src/llvm/lib/SYCLLowerIR/SYCLCreateNVVMAnnotations.cpp
/Users/runner/work/llvm/llvm/src/llvm/lib/SYCLLowerIR/SYCLCreateNVVMAnnotations.cpp:62:10: error: no viable conversion from returned value of type 'array<optional<size_t>, [...]>' to function return type 'array<optional<uint64_t>, [...]>'
  return Ops;
         ^~~
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/array:129:29: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'std::array<std::optional<size_t>, 3>' (aka 'array<optional<unsigned long>, 3>') to 'const std::array<std::optional<unsigned long long>, 3> &' for 1st argument
struct _LIBCPP_TEMPLATE_VIS array
                            ^
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/array:129:29: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'std::array<std::optional<size_t>, 3>' (aka 'array<optional<unsigned long>, 3>') to 'std::array<std::optional<unsigned long long>, 3> &&' for 1st argument
1 error generated.

frasercrmck · 2024-08-06T09:10:57Z

@frasercrmck We are seeing macos post commit failures, can you please take a look?

https://github.com/intel/llvm/actions/runs/10253109905/job/28365021433

FAILED: lib/SYCLLowerIR/CMakeFiles/LLVMSYCLLowerIR.dir/SYCLCreateNVVMAnnotations.cpp.o 
ccache /Applications/Xcode_14.2.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/Users/runner/work/llvm/llvm/build/lib/SYCLLowerIR -I/Users/runner/work/llvm/llvm/src/llvm/lib/SYCLLowerIR -I/Users/runner/work/llvm/llvm/build/include -I/Users/runner/work/llvm/llvm/src/llvm/include -I/Users/runner/work/llvm/llvm/src/llvm/projects/vc-intrinsics/GenXIntrinsics/include -I/Users/runner/work/llvm/llvm/build/projects/vc-intrinsics/GenXIntrinsics/include -I/Users/runner/work/llvm/llvm/build/_deps/vc-intrinsics-src/GenXIntrinsics/include -I/Users/runner/work/llvm/llvm/build/_deps/vc-intrinsics-build/GenXIntrinsics/lib/GenXIntrinsics/../../include -I/Users/runner/work/llvm/llvm/build/_deps/vc-intrinsics-build/GenXIntrinsics/include -isystem /usr/local/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -O3 -DNDEBUG -std=c++17 -isysroot /Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -mmacosx-version-min=12.7  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT lib/SYCLLowerIR/CMakeFiles/LLVMSYCLLowerIR.dir/SYCLCreateNVVMAnnotations.cpp.o -MF lib/SYCLLowerIR/CMakeFiles/LLVMSYCLLowerIR.dir/SYCLCreateNVVMAnnotations.cpp.o.d -o lib/SYCLLowerIR/CMakeFiles/LLVMSYCLLowerIR.dir/SYCLCreateNVVMAnnotations.cpp.o -c /Users/runner/work/llvm/llvm/src/llvm/lib/SYCLLowerIR/SYCLCreateNVVMAnnotations.cpp
/Users/runner/work/llvm/llvm/src/llvm/lib/SYCLLowerIR/SYCLCreateNVVMAnnotations.cpp:62:10: error: no viable conversion from returned value of type 'array<optional<size_t>, [...]>' to function return type 'array<optional<uint64_t>, [...]>'
  return Ops;
         ^~~
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/array:129:29: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'std::array<std::optional<size_t>, 3>' (aka 'array<optional<unsigned long>, 3>') to 'const std::array<std::optional<unsigned long long>, 3> &' for 1st argument
struct _LIBCPP_TEMPLATE_VIS array
                            ^
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/array:129:29: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'std::array<std::optional<size_t>, 3>' (aka 'array<optional<unsigned long>, 3>') to 'std::array<std::optional<unsigned long long>, 3> &&' for 1st argument
1 error generated.

Whoops, sorry. See #14964

sarnex · 2024-08-06T14:52:52Z

Thanks!

This patch refactors the way we lower SYCL attributes and properties to NVVM annotations, through function attributes and metadata. It unifies the flow better with the SPIR-V paths at the same time. Previously we had: 1. Clang handling function attributes in two places: 1. CodeGenFunction did generic lowering of attributes to function metadata 2. NVPTXTargetCodeGenInfo did its own additional lowering of attributes to NVVM annotations 2. Kernel properties being handled in clang. NVPTXTargetCodeGenInfo lowered kernel properties, which had already been converted to function attributes, to NVVM annotations. 3. Kernel properties for HIP/CUDA *not* being turned into function metadata. Because function metadata is how the ComputeModuleRuntimeInfo library creates runtime info, this meant that target backends couldn't act on properties, as they were lost during lowering. Fundamentally, clang is not the correct place for handling SYCL kernel properties as it unnecessarily touches the front-end for what is really library code. With this patch, we now have: 1. Clang handles function attributes in one place, lowering to function metadata 2. SYCLLowerIR lowers kernel properties to the same function metadata: now working for HIP and CUDA kernels too. Because function metadata is consistently present for kernel properties, they behave as expected on HIP/CUDA targets now. 3. A new pass converts function metadata to NVVM annotations, replacing the code we've taken out of clang.

frasercrmck requested review from a team as code owners July 18, 2024 16:44

frasercrmck requested a review from dm-vodopyanov July 18, 2024 16:44

frasercrmck had a problem deploying to WindowsCILock July 18, 2024 16:45 — with GitHub Actions Error

remove unrelated code

84b0d76

frasercrmck had a problem deploying to WindowsCILock July 18, 2024 16:47 — with GitHub Actions Failure

frasercrmck commented Jul 18, 2024

View reviewed changes

llvm/lib/SYCLLowerIR/TargetHelpers.cpp Outdated Show resolved Hide resolved

frasercrmck temporarily deployed to WindowsCILock July 18, 2024 19:50 — with GitHub Actions Inactive

EwanC reviewed Jul 19, 2024

View reviewed changes

sycl/test-e2e/Basic/work_group_size_prop.cpp Outdated Show resolved Hide resolved

undo unintentional change

4c71d9f

frasercrmck had a problem deploying to WindowsCILock July 22, 2024 10:03 — with GitHub Actions Failure

EwanC approved these changes Jul 22, 2024

View reviewed changes

frasercrmck had a problem deploying to WindowsCILock July 22, 2024 10:49 — with GitHub Actions Error

update and fix tests

a273f33

frasercrmck had a problem deploying to WindowsCILock July 22, 2024 11:35 — with GitHub Actions Failure

frasercrmck temporarily deployed to WindowsCILock July 22, 2024 12:21 — with GitHub Actions Inactive

dm-vodopyanov approved these changes Jul 22, 2024

View reviewed changes

fix llvm tests

53daf65

frasercrmck had a problem deploying to WindowsCILock July 22, 2024 14:01 — with GitHub Actions Failure

frasercrmck had a problem deploying to WindowsCILock July 22, 2024 14:44 — with GitHub Actions Error

fix pass name

968ff92

frasercrmck had a problem deploying to WindowsCILock July 22, 2024 15:12 — with GitHub Actions Failure

sarnex approved these changes Jul 22, 2024

View reviewed changes

sarnex requested a review from a team July 22, 2024 15:13

frasercrmck had a problem deploying to WindowsCILock July 22, 2024 16:11 — with GitHub Actions Error

fix test triple

63edd00

frasercrmck temporarily deployed to WindowsCILock July 22, 2024 16:17 — with GitHub Actions Inactive

frasercrmck temporarily deployed to WindowsCILock July 22, 2024 18:18 — with GitHub Actions Inactive

frasercrmck mentioned this pull request Jul 23, 2024

[SYCL] Add max work-group size kernel properties #14518

Merged

elizabethandrews approved these changes Jul 23, 2024

View reviewed changes

smanna12 approved these changes Jul 23, 2024

View reviewed changes

frasercrmck mentioned this pull request Jul 25, 2024

[SYCLLowerIR] Remove !amdgcn.annotations metadata #14713

Merged

frasercrmck added 3 commits July 29, 2024 20:04

Merge branch 'sycl' into sycl-nvptx-hip-annotations-pass

2ba56fd

Merge remote-tracking branch 'origin/sycl' into sycl-nvptx-hip-annota…

510afe8

…tions-pass

fix conflict

6f4700f

frasercrmck temporarily deployed to WindowsCILock July 29, 2024 19:19 — with GitHub Actions Inactive

frasercrmck temporarily deployed to WindowsCILock July 29, 2024 22:35 — with GitHub Actions Inactive

Merge remote-tracking branch 'origin/sycl' into sycl-nvptx-hip-annota…

35da939

…tions-pass

frasercrmck temporarily deployed to WindowsCILock August 5, 2024 09:59 — with GitHub Actions Inactive

frasercrmck temporarily deployed to WindowsCILock August 5, 2024 10:43 — with GitHub Actions Inactive

bader merged commit 3d73d9b into intel:sycl Aug 5, 2024
14 checks passed

frasercrmck deleted the sycl-nvptx-hip-annotations-pass branch August 5, 2024 17:04

againull mentioned this pull request Feb 27, 2025

[SYCL][Doc] Release Notes for Nov'24 release #17179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NVPTX][AMDGPU] Move annotation creation out of clang #14634

[NVPTX][AMDGPU] Move annotation creation out of clang #14634

Uh oh!

frasercrmck commented Jul 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

EwanC left a comment

Uh oh!

dm-vodopyanov left a comment

Uh oh!

sarnex left a comment

Uh oh!

bader commented Jul 22, 2024

Uh oh!

bader commented Jul 29, 2024

Uh oh!

frasercrmck commented Jul 29, 2024

Uh oh!

frasercrmck commented Aug 5, 2024

Uh oh!

Uh oh!

sarnex commented Aug 5, 2024 •

edited

Loading

Uh oh!

frasercrmck commented Aug 6, 2024

Uh oh!

sarnex commented Aug 6, 2024

Uh oh!

Uh oh!

[NVPTX][AMDGPU] Move annotation creation out of clang #14634

[NVPTX][AMDGPU] Move annotation creation out of clang #14634

Uh oh!

Conversation

frasercrmck commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EwanC left a comment

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov left a comment

Choose a reason for hiding this comment

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Jul 22, 2024

Uh oh!

bader commented Jul 29, 2024

Uh oh!

frasercrmck commented Jul 29, 2024

Uh oh!

frasercrmck commented Aug 5, 2024

Uh oh!

Uh oh!

sarnex commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frasercrmck commented Aug 6, 2024

Uh oh!

sarnex commented Aug 6, 2024

Uh oh!

Uh oh!

frasercrmck commented Jul 18, 2024 •

edited

Loading

sarnex commented Aug 5, 2024 •

edited

Loading