[libspirv] Replace libclc-remangler with SYCLRemangleLibspirvPass#21588
[libspirv] Replace libclc-remangler with SYCLRemangleLibspirvPass#21588wenju-he wants to merge 14 commits intointel:syclfrom
Conversation
Remove libclc-remangler tool and replace it with SYCLBuiltinRemanglePass that remangles SPIR-V built-ins in SYCL user device code to match OpenCL C mangling in libspirv. The pass is inverse of what libclc-remangler did. Motivation: - libclc-remangler executable doesn't fit in runtime build. - libclc-remangler can't justify as a standalone tool in llvm/tools. Changes: - Add SYCLBuiltinRemanglePass. It uses Itanium demangler and adapts SPIR type system and SPIR mangler from SPIRV-LLVM-Translator. - Type transformations: - long long -> long (OpenCL has no long long) - long -> int (Windows or 32-bit only) - signed char -> char - char -> unsigned char (only if char is signed on the host) - _Float16 -> half (with a few w/a for native-cpu libdevice) - Address space adjustments if target's default addrspace is private. - libspirv is now linked into user code after the optimization pipeline by -mlink-builtin-bitcode-postopt flag since the linking must be after remangling. In the future, the linking will be moved to LTO. - Changes in _CLC_DEFINE_MIPMAP_BINDLESS_READS_BUILTIN also exposed a bug in libclc-remangler which would remangle _Z30__spirv_ImageSampleExplicitLodImDv4_cDv3_fET0_T_T1_iS4_S4_ (correct) to _Z30__spirv_ImageSampleExplicitLodImDv4_aDv3_fET0_T_T1_iS1_S1_ This bug was hidden since correct symbol was hardcoded in the source. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
| When the SYCL compiler is in device mode and targeting the NVPTX backend, the | ||
| compiler exposes NVPTX builtins supported by clang. | ||
| incompatible libclc built-ins. When building a SYCL application targeting the | ||
| CUDA backend, the SYCLBuiltinRemangle pass remangles SPIR-V builtins in device |
There was a problem hiding this comment.
I'm wondering could we make clang emit calls to these functions using the correct name mangling from the start instead of re-mangling as a pass?
I was thinking a function attribute that allows to switch the name mangling to be OpenCL compatible could theoretically work for this. We would need something similar if we wanted to author libclc sources in C++.
There was a problem hiding this comment.
I'm wondering could we make clang emit calls to these functions using the correct name mangling from the start instead of re-mangling as a pass?
This is interesting idea, but I don't know about how to implement the automatic mapping.
We would need something similar if we wanted to author libclc sources in C++.
Implement libspirv in C++ and compile in sycl device mode like libdevice does can indeed avoid the remangling issue. However, one problem is that it will be very difficult to share implementation between libclc OpenCL and libspirv builtin wrappers, since eventually the sharing requires a stable interface which SPV-IR intends to be.
elizabethandrews
left a comment
There was a problem hiding this comment.
FE changes just add a pass. FE changes LGTM. I have not reviewed the functionality of the pass.
…RemangleLibspirvPass Not inlined __clc_ functions are not remangled because we only have a single clc library that has OpenCL C mangling.
| if (AS == 1) | ||
| Attr = SPIR::ATTR_GLOBAL; | ||
| else if (AS == 2) | ||
| Attr = SPIR::ATTR_CONSTANT; | ||
| else if (AS == 3) | ||
| Attr = SPIR::ATTR_LOCAL; | ||
| else if (AS == 4) | ||
| Attr = SPIR::ATTR_GENERIC_EXPLICIT; |
There was a problem hiding this comment.
what is AS == 5 or something? should we add assert or llvm_unreachable?
There was a problem hiding this comment.
what is
AS == 5or something? should we add assert or llvm_unreachable?
This will be handled in follow-up PR since it will cause llvm-diff change to remangled libspirv. Current address space mapping is incorrect for non-spirv targets. The issues exists before this PR.
There was a problem hiding this comment.
could you please add TODO comment and maybe point to github issue?
There was a problem hiding this comment.
could you please add TODO comment and maybe point to github issue?
added comment linking to #21664
| // Skip __clc_ functions. If we remangle them, it should be done in clc | ||
| // library and it requires creating multiple variants of clc libaries, | ||
| // with mangling scheme aligning with the corresponding libspirv remangling. | ||
| if (F.isDeclaration() && F.getName().contains("__clc_")) |
There was a problem hiding this comment.
this condition skips only __clc_ declarations. Should we also skip definitions?
There was a problem hiding this comment.
this condition is correct. Only __clc_ declarations from CLC library, which is built before libspirv build, should be ignored.
__clc_ definitions in libspirv should be remangled because they are not from CLC library. Actually libspirv shouldn't provide any __clc_ definitions, but this is just current status.
I think it is feasible to skip __clc_ definitions here since they are not public APIs, but I'd not do so in this PR to avoid llvm-diff change from old remangled libspirv prior to this PR.
There was a problem hiding this comment.
I see. could you please clarify the comment then. currently it is saying to skip __clc_ functions, but without details you provided in the reply, it is not clear, why we are skipping only declarations, but not definitions.
There was a problem hiding this comment.
libspirv should not contain _clc definitions. Adding a comment to justify a mistake, only to remove it later, don't have much value.
| MangleVisitor Visitor(MangledName, Stream, 1); | ||
| for (const auto &TemplateArg : TemplateArgs) { | ||
| MangleError Err = TemplateArg->accept(&Visitor); | ||
| if (Err != MANGLE_SUCCESS) |
There was a problem hiding this comment.
The only value of MangleError is MANGLE_SUCCESS. What is the scenario, where Err would be different? Should more MangleError cases be added to enum, like MANGLE_FAILURE and then we should use it somewhere?
There was a problem hiding this comment.
Current error macros in llvm-spirv mangler don't suite here. I kept MANGLE_SUCCESS here for API consistency. Should we just delete MANGLE_SUCCESS?
There was a problem hiding this comment.
yes, I think it would be fine to delete, if it is not used meaningfully
There was a problem hiding this comment.
yes, I think it would be fine to delete, if it is not used meaningfully
deleted MANGLE_SUCCESS
| // CloneOriginal recreates it so callers using OpenCL type names still link. | ||
| // Clone source is found by applying CloneOriginal's map to original name: | ||
| // char -> signed char (signed) / unsigned char (unsigned) | ||
| // _Float16 -> half, long -> int (L32) or long long (L64), |
There was a problem hiding this comment.
based on further comments, there is no CloneOriginal entry for 'half'
| // _Float16 -> half, long -> int (L32) or long long (L64), | |
| // long -> int (L32) or long long (L64), |
There was a problem hiding this comment.
based on further comments, there is no CloneOriginal entry for 'half'
done, thanks
YuriPlyakhin
left a comment
There was a problem hiding this comment.
4 comments left to address for files owned by me.
Remove libclc-remangler tool and replace it with SYCLRemangleLibspirvPass
that remangles OpenCL C mangling in libspirv to match with SYCL mangling.
The pass ports the functionalities in the tool into compile pipeline.
Motivation:
Functional changes:
libspirv variants are created without separate remangling step.
llvm-diff changes to remangled libspirv:
a single clc library that defaults to OpenCL C mangling.
Previously, these were called only once and inlined before remangling.
Now, because remangling can generate two caller variants, the _clc
function is called twice, causing the inliner to skip them.
This bug was hidden since correct symbol was hardcoded in the source.
_Z23__spirv_ImageArrayWriteIyiiEvT_T0_T1_i -> Z23__spirv_ImageArrayWriteIyiiEvT_T0_iT1;
_Z23__spirv_ImageArrayWriteImiiEvT_T0_T1_i -> Z23__spirv_ImageArrayWriteImiiEvT_T0_iT1.