-
Notifications
You must be signed in to change notification settings - Fork 794
[SYCL] Use SPIR-V built-in function call for all targets and add BuiltIn to the name
#19359
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
Conversation
…ltIn` to the name Before this PR, SPIR-V built-in is represented as global variable for SPIR/SPIR-V targets and as function call for other targets in include/sycl/__spirv/spirv_vars.hpp. According to https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/docs/SPIRVRepresentationInLLVM.rst, SPIR-V built-in variable can be mapped to either function call or global variable. So function call representation should work for SPIR-V target. Add `BuiltIn` to the name to align with SPIR-V friendly IR.
This fixes pre-commit CI fail in intel/llvm#19359
…#148567) The mapping ensures the function is lowered to SPIR-V built-in variables in SPIR-V. This can fix pre-commit CI fail in intel/llvm#19359 Also add BuiltIn to SPIR-V Builtin function name in __clang_spirv_builtins.h to align with https://github.com/llvm/llvm-project/blob/main/llvm/docs/SPIRVUsage.rst#builtin-variables
…n variables (#148567) The mapping ensures the function is lowered to SPIR-V built-in variables in SPIR-V. This can fix pre-commit CI fail in intel/llvm#19359 Also add BuiltIn to SPIR-V Builtin function name in __clang_spirv_builtins.h to align with https://github.com/llvm/llvm-project/blob/main/llvm/docs/SPIRVUsage.rst#builtin-variables
… (#148567) The mapping ensures the function is lowered to SPIR-V built-in variables in SPIR-V. This can fix pre-commit CI fail in intel#19359 (cherry picked from commit 64205ad) This is a partial cherry-pick since 27c9b55 isn't in intel/llvm yet.
|
kindly ping @intel/llvm-reviewers-runtime @intel/dpcpp-esimd-reviewers @intel/dpcpp-sanitizers-review for review |
|
|
||
|
|
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.
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.
done, thanks @uditagarwal97
sarnex
left a comment
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 minus one question
| DEVICE_EXTERN_C_INLINE | ||
| int rand() { | ||
| size_t gid = | ||
| #if defined(__NVPTX__) || defined(__AMDGCN__) |
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.
The PR description says
According to https://github.com/llvm/llvm-project/blob/main/llvm/docs/SPIRVUsage.rst, SPIR-V built-in variable can be mapped to either function call or global variable. So function call representation should work for SPIR-V target as well.
So it sounds like both using a global and a function are valid in SPIR-V friendly IR.
Is there some benefit we get from changing it from a global variable to a function? I assume there is, but please add it to the PR description.
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.
Is there some benefit we get from changing it from a global variable to a function? I assume there is, but please add it to the PR description.
done, add following to PR description:
The benefit of choosing function call over global variable is that
function call works better for AOT targets that bypass SPIR-V. It is
easier for these targets to implements SPIR-V built-in function via
built-in modules like libclc rather than special handling for lowering
global variable in a custom pass.
zhaomaosu
left a comment
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.
sanitizer part lgtm
…_BuiltIn* global variables
MrSidims
left a comment
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.
Barrier pass modification is LGTM
sarnex
left a comment
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 as in my feedback was addressed, thanks!
|
@intel/llvm-gatekeepers please merge, thanks |
Before this PR, SPIR-V built-in is represented as global variable for SPIR/SPIR-V targets and as function call for other targets in include/sycl/__spirv/spirv_vars.hpp.
According to https://github.com/llvm/llvm-project/blob/main/llvm/docs/SPIRVUsage.rst, SPIR-V built-in variable can be mapped to either function call or global variable. So function call representation should work for SPIR-V target as well.
The benefit of choosing function call over global variable is that
function call works better for AOT targets that bypass SPIR-V. It is
easier for these targets to implements SPIR-V built-in function via
built-in modules like libclc rather than special handling for lowering
global variable in a custom pass.
Add
BuiltInto the name to align with SPIR-V friendly IR and global variable name.