[clang][Driver] Add HIPAMD Driver support for AMDGCN flavoured SPIR-V #95061
Conversation
| @@ -907,7 +907,8 @@ void CodeGenModule::Release() { | |||
| if (Context.getTargetInfo().getTriple().isWasm()) | |||
| EmitMainVoidAlias(); | |||
|
|
|||
| if (getTriple().isAMDGPU()) { | |||
| if (getTriple().isAMDGPU() || | |||
| (getTriple().isSPIRV() && getTriple().getVendor() == llvm::Triple::AMD)) { | |||
There was a problem hiding this comment.
I'm wondering if we should add isAMD to llvm::Triple or something.
There was a problem hiding this comment.
I didn't know the vendor got used for anything.
There was a problem hiding this comment.
I'm wondering if we should add
isAMDtollvm::Tripleor something.
We do have isAMDGCN, but that reflects the Arch. I guess it might be convenient sugar to have a predicate on vendors as well? I don't find the manual check excessively offensive FWIW.
There was a problem hiding this comment.
I didn't know the vendor got used for anything.
This matches how we've documented it when we added the AMDGCN flavoured SPIR-V, and seemed to reflect the idea that this is SPIRV(64) with customisations for AMD as the vendor; do you think it is or can become problematic?
| auto OffloadArchs = Args.getAllArgValues(options::OPT_offload_arch_EQ); | ||
| if (llvm::find(OffloadArchs, "amdgcnspirv") != OffloadArchs.cend()) { | ||
| if (OffloadArchs.size() == 1) | ||
| return llvm::Triple("spirv64-amd-amdhsa"); |
There was a problem hiding this comment.
Use a toolchain with spirv64 as triple will cause trouble for us to support mixed amdgcn and spirv fat binaries, which is critical for us.
Better to take the approach similar to #75357, i.e. treat spirv as a processor of amgcn triple, so that we can use HIPAMD toolchain for both spirv and real amdgcn processor.
There was a problem hiding this comment.
Thanks for the pointer, much appreciated! I will revisit this/take your suggestion, but for the initial experimental support it was deemed acceptable to carry this temporary limitation (unless you strongly object, of course).
There was a problem hiding this comment.
I am OK to commit this since the command line option won't change so users are not affected.
…llvm#95061) This patch augments the HIPAMD driver to allow it to target AMDGCN flavoured SPIR-V compilation. It's mostly straightforward, as we re-use some of the existing SPIRV infra, however there are a few notable additions: - we introduce an `amdgcnspirv` offload arch, rather than relying on using `generic` (this is already fairly overloaded) or simply using `spirv` or `spirv64` (we'll want to use these to denote unflavoured SPIRV, once we bring up that capability) - initially it is won't be possible to mix-in SPIR-V and concrete AMDGPU targets, as it would require some relatively intrusive surgery in the HIPAMD Toolchain and the Driver to deal with two triples (`spirv64-amd-amdhsa` and `amdgcn-amd-amdhsa`, respectively) - in order to retain user provided compiler flags and have them available at JIT time, we rely on embedding the command line via `-fembed-bitcode=marker`, which the bitcode writer had previously not implemented for SPIRV; we only allow it conditionally for AMDGCN flavoured SPIRV, and it is handled correctly by the Translator (it ends up as a string literal) Once the SPIRV BE is no longer experimental we'll switch to using that rather than the translator. There's some additional work that'll come via a separate PR around correctly piping through AMDGCN's implementation of `printf`, for now we merely handle its flags correctly.
| "--spirv-allow-unknown-intrinsics", | ||
| "--spirv-lower-const-expr", | ||
| "--spirv-preserve-auxdata", | ||
| "--spirv-debug-info-version=nonsemantic-shader-200"}; |
There was a problem hiding this comment.
Recently I've found this patch in gitlog and was intrigued, does this line mean, that AMD driver supports KhronosGroup/SPIRV-Registry#186 ? Just for my curiosity. It may also make me push the instruction set for ratification sooner :)
| llvm::opt::ArgStringList TrArgs{ | ||
| "--spirv-max-version=1.6", | ||
| "--spirv-ext=+all", | ||
| "--spirv-allow-extra-diexpressions", |
There was a problem hiding this comment.
@AlexVlx nit: if generation of NonSemantic.Shader.DebugInfo.200 is turned on - this option is not needed as the extended instruction already adds all DWARF expressions (including LLVM-specific expressions).
This patch augments the HIPAMD driver to allow it to target AMDGCN flavoured SPIR-V compilation. It's mostly straightforward, as we re-use some of the existing SPIRV infra, however there are a few notable additions:
amdgcnspirvoffload arch, rather than relying on usinggeneric(this is already fairly overloaded) or simply usingspirvorspirv64(we'll want to use these to denote unflavoured SPIRV, once we bring up that capability)spirv64-amd-amdhsaandamdgcn-amd-amdhsa, respectively)-fembed-bitcode=marker, which the bitcode writer had previously not implemented for SPIRV; we only allow it conditionally for AMDGCN flavoured SPIRV, and it is handled correctly by the Translator (it ends up as a string literal), but a glance from someone working on the SPIRV backend would be appreciatedOnce the SPIRV BE is no longer experimental we'll switch to using that rather than the translator. There's some additional work that'll come via a separate PR around correctly piping through AMDGCN's implementation of
printf, for now we merely handle its flags correctly.