-
Notifications
You must be signed in to change notification settings - Fork 759
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
[SYCL] Change SPIR-V Enum token type from unsigned int to int for barrier builtins #17438
[SYCL] Change SPIR-V Enum token type from unsigned int to int for barrier builtins #17438
Conversation
…rier builtins Motivation is unifying SPIR-V builtin mangling to enhance SYCL AOT support for backend targets that bypass SPIR-V generation. Changing to signed int type aligns with * SPV-IR output of llvm-spirv translator. * Default underlying type of enum being int, e.g. enum defined in tablegened LLVM SPIR-V backend header and standard SPIR-V header. Changes are mainly made to following places: * sycl/include/sycl/__spirv/spirv_ops.hpp * clang/lib/Sema/SPIRVBuiltins.td * libclc/libspirv
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.
SYCL-JIT changes LGTM.
…ups builtins Same motivatoin as in PR intel#17438
…ups builtins Motivation is the same as PR intel#17438, i.e. unifying SPIR-V builtin mangling to enhance SYCL AOT support for backend targets that bypass SPIR-V generation.
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.
ptx/gcn changes LGTM
sycl/source/spirv_ops.cpp
Outdated
__SYCL_EXPORT void __spirv_ControlBarrier(__spv::Scope Execution, | ||
__spv::Scope Memory, | ||
uint32_t Semantics) noexcept { | ||
__SYCL_EXPORT void __spirv_ControlBarrier(int32_t Execution, int32_t Memory, |
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.
These are for the old host device support, I'd advise to not touch these as it creates an ABI break and that's only for the host.
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.
thanks @Naghasan, reverted the ABI change
…PIRVBuiltins.td Also change scope and memory semantics type from Enum from int, similar as PR intel#17438 volatile is removed from pointer parameter type, to align with SVP-IR and clang/lib/Sema/SPIRVBuiltins.td.
…PIRVBuiltins.td Also change scope and memory semantics type from Enum from int, similar as PR intel#17438 volatile is removed from pointer parameter type, to align with SVP-IR and clang/lib/Sema/SPIRVBuiltins.td.
…PIRVBuiltins.td Also change scope and memory semantics type from Enum from int, similar as PR intel#17438 volatile is removed from pointer parameter type, to align with SVP-IR and clang/lib/Sema/SPIRVBuiltins.td.
…PIRVBuiltins.td Change scope and memory semantics type from Enum from int, similar as in PR intel#17438. volatile is removed from function parameter pointer type, to align with SVP-IR and clang/lib/Sema/SPIRVBuiltins.td.
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.
Changes in sycl/test/check_device_code/
LGTM.
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.
Looks good on "sanitizer_defs.hpp"
kindly ping @intel/dpcpp-tools-reviewers, @intel/dpcpp-cfe-reviewers for review |
// TODO: Allow enum flags instead of UInt ? | ||
// TODO: We should enforce that the UInt must be a literal. | ||
def : SPVBuiltin<name, [Void, UInt, UInt, UInt], Attr.Convergent>; | ||
// TODO: Allow enum flags instead of Int ? |
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.
Should this also have a clang test?
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.
updated clang test in 49d9641
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.
libclc LGTM
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.
SYCLLowerIR LGTM.
@intel/llvm-gatekeepers please merge, thanks |
Motivation is unifying SPIR-V builtin mangling to enhance SYCL AOT support for backend targets that bypass SPIR-V generation. Changing to signed int type aligns with
Changes are mainly made to following places: