[SYCL] Move Subgroup*INTEL SPIR-V built-in declarations to clang SPIRVBuiltins.td #17732
Conversation
…VBuiltins.td Motivation is similar to c040f9a : unify with SPV-IR mangling. SubgroupBlockReadINTEL is not handled in this PR. SPV-IR appends return type to mangled function name. It might be simpler for SYCL header to keep current mangling.
frasercrmck
left a comment
frasercrmck
left a comment
There was a problem hiding this comment.
libspirv LGTM, thank you.
To be honest I'm not sure why some of this amdgcn-amdhsa stuff is target-specific if they're just using generic SPIR-V builtins. I'd have thought other targets may want to make use of them.
I'm also not sure if/why we need to "declare" the SPIR-V builtins in places if they're going to be coming from clang anyway.
Those two are questions for another PR but worth looking into.
| __CLC_DECLARE_SHUFFLES(unsigned long, m); | ||
| __CLC_DECLARE_SHUFFLES(double, d); | ||
| #define __CLC_DECLARE_SHUFFLES(TYPE) \ | ||
| _CLC_OVERLOAD _CLC_DECL TYPE __spirv_SubgroupShuffleINTEL( \ |
There was a problem hiding this comment.
I'm not sure if we can remove implementation of existing built-ins. Won't this be an ABI breaking change? The main question is: can we link static libraries produced by the old compiler using new compiler?
There was a problem hiding this comment.
Yes it would be an ABI breakage for device code and we can't link link static libraries produced by the old compiler using new compiler.
Should I keep all the existing implementation?
We probably need to add note for either removal or deprecation in upcoming release notes.
There was a problem hiding this comment.
Should I keep all the existing implementation?
Yes. We should keep old implementation to maintain stable ABI as required by the Contribution guidelines.
I'm not sure about release notes. This is implementation detail, which is not visible to users. We are allowed to break ABI with major version changes.
@AlexeySachkov, FYI.
There was a problem hiding this comment.
added back old implementations.
clang/test/CodeGenSPIRV/spirv-builtin-lookup-intel-subgroup.cpp
Outdated
Show resolved
Hide resolved
Thanks for the review. I'll follow-up this issue later.
removed in c750ebf |
|
thank you @uwedolinsky |
hvdijk
left a comment
hvdijk
left a comment
There was a problem hiding this comment.
The NativeCPU part looks good, thanks.
|
@intel/llvm-reviewers-runtime please review, thanks. |
7 participants
Motivation is similar to c040f9a : unify with SPV-IR mangling.
SubgroupBlockReadINTEL is not handled in this PR. SPV-IR appends return type to mangled function name. It might be simpler for SYCL header to keep current mangling.