Skip to content

[NFC][Driver][HIP] Fix mixing amdgcnspirv and gfxXXX via --offload-arch #133024

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

Merged
merged 2 commits into from
Mar 26, 2025

Conversation

AlexVlx
Copy link
Contributor

@AlexVlx AlexVlx commented Mar 26, 2025

Due to amdgcnspirv piggybacking on the HIPAMDToolchain, it loses its immediately apparent SPIR-Vness, which makes the Driver want to go all the way to Assembly emmission. This was problematic as we were trying to llvm-link SPIR-V, before trying to translate it. This patch ensures that we do the right thing and stop at bitcode emission if we are mixing amdgcnspirv and concrete targets.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Mar 26, 2025
@AlexVlx AlexVlx requested review from jhuber6 and yxsamliu March 26, 2025 01:04
@llvmbot
Copy link
Member

llvmbot commented Mar 26, 2025

@llvm/pr-subscribers-clang

Author: Alex Voicu (AlexVlx)

Changes

Due to amdgcnspirv piggybacking on the HIPAMDToolchain, it loses its immediately apparent SPIR-Vness, which makes the Driver want to go all the way to Assembly emmission. This was problematic as we were trying to llvm-link SPIR-V, before trying to translate it. This patch ensures that we do the right thing and stop at bitcode emission if we are mixing amdgcnspirv and concrete targets.


Full diff: https://github.com/llvm/llvm-project/pull/133024.diff

1 Files Affected:

  • (modified) clang/lib/Driver/Driver.cpp (+5-2)
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 056bfcf1b739a..07e36ea2efba4 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -3750,9 +3750,12 @@ class OffloadingActionBuilder final {
             // compiler phases, including backend and assemble phases.
             ActionList AL;
             Action *BackendAction = nullptr;
-            if (ToolChains.front()->getTriple().isSPIRV()) {
+            if (ToolChains.front()->getTriple().isSPIRV() ||
+                (ToolChains.front()->getTriple().isAMDGCN() &&
+                 GpuArchList[I] == StringRef("amdgcnspirv"))) {
               // Emit LLVM bitcode for SPIR-V targets. SPIR-V device tool chain
-              // (HIPSPVToolChain) runs post-link LLVM IR passes.
+              // (HIPSPVToolChain or HIPAMDToolChain) runs post-link LLVM IR
+              // passes.
               types::ID Output = Args.hasArg(options::OPT_S)
                                      ? types::TY_LLVM_IR
                                      : types::TY_LLVM_BC;

@llvmbot
Copy link
Member

llvmbot commented Mar 26, 2025

@llvm/pr-subscribers-clang-driver

Author: Alex Voicu (AlexVlx)

Changes

Due to amdgcnspirv piggybacking on the HIPAMDToolchain, it loses its immediately apparent SPIR-Vness, which makes the Driver want to go all the way to Assembly emmission. This was problematic as we were trying to llvm-link SPIR-V, before trying to translate it. This patch ensures that we do the right thing and stop at bitcode emission if we are mixing amdgcnspirv and concrete targets.


Full diff: https://github.com/llvm/llvm-project/pull/133024.diff

1 Files Affected:

  • (modified) clang/lib/Driver/Driver.cpp (+5-2)
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 056bfcf1b739a..07e36ea2efba4 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -3750,9 +3750,12 @@ class OffloadingActionBuilder final {
             // compiler phases, including backend and assemble phases.
             ActionList AL;
             Action *BackendAction = nullptr;
-            if (ToolChains.front()->getTriple().isSPIRV()) {
+            if (ToolChains.front()->getTriple().isSPIRV() ||
+                (ToolChains.front()->getTriple().isAMDGCN() &&
+                 GpuArchList[I] == StringRef("amdgcnspirv"))) {
               // Emit LLVM bitcode for SPIR-V targets. SPIR-V device tool chain
-              // (HIPSPVToolChain) runs post-link LLVM IR passes.
+              // (HIPSPVToolChain or HIPAMDToolChain) runs post-link LLVM IR
+              // passes.
               types::ID Output = Args.hasArg(options::OPT_S)
                                      ? types::TY_LLVM_IR
                                      : types::TY_LLVM_BC;

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems in line with the weird workarounds we already use.

@AlexVlx AlexVlx merged commit 1a7402d into llvm:main Mar 26, 2025
11 checks passed
@AlexVlx AlexVlx deleted the mixed_amdgcnspirv_concrete_was_broken branch March 26, 2025 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants