[Clang] Fix new driver device only compilation for amdgcnspirv target

[jhuber6]

Summary:
This is broken with the current target because it was not bundling the
output as HIP likes and this would fail if targeting both at the same
time.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Joseph Huber (jhuber6)

Changes

Summary:
This is broken with the current target because it was not bundling the
output as HIP likes and this would fail if targeting both at the same
time.

---

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

2 Files Affected:

• (modified) clang/lib/Driver/Driver.cpp (+6-2)
• (modified) clang/test/Driver/hip-phases.hip (+23)

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index ff2f92d1a94c8..efa6a8a03c696 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -4886,6 +4886,9 @@ Action *Driver::BuildOffloadingActions(Compilation &C,
     // individually.
     for (Action *&A : DeviceActions) {
       if ((A->getType() != types::TY_Object &&
+           !(A->getOffloadingToolChain()->getTriple().getOS() ==
+                 llvm::Triple::OSType::AMDHSA &&
+             A->getOffloadingToolChain()->getTriple().isSPIRV()) &&
            A->getType() != types::TY_LTO_BC) ||
           !HIPNoRDC || !offloadDeviceOnly())
         continue;
@@ -4941,8 +4944,9 @@ Action *Driver::BuildOffloadingActions(Compilation &C,
     // fatbinary for each translation unit, linking each input individually.
     Action *FatbinAction =
         C.MakeAction<LinkJobAction>(OffloadActions, types::TY_HIP_FATBIN);
-    DDep.add(*FatbinAction, *C.getSingleOffloadToolChain<Action::OFK_HIP>(),
-             nullptr, Action::OFK_HIP);
+    DDep.add(*FatbinAction,
+             *C.getOffloadToolChains<Action::OFK_HIP>().first->second, nullptr,
+             Action::OFK_HIP);
   } else {
     // Package all the offloading actions into a single output that can be
     // embedded in the host and linked.
diff --git a/clang/test/Driver/hip-phases.hip b/clang/test/Driver/hip-phases.hip
index d8a58b78d6d5c..f4d3e9d6043d9 100644
--- a/clang/test/Driver/hip-phases.hip
+++ b/clang/test/Driver/hip-phases.hip
@@ -675,3 +675,26 @@
 // DEVICE-ONLY-NEXT: 2: compiler, {1}, ir, (device-hip, gfx90a)
 // DEVICE-ONLY-NEXT: 3: backend, {2}, ir, (device-hip, gfx90a)
 // DEVICE-ONLY-NEXT: 4: offload, "device-hip (amdgcn-amd-amdhsa:gfx90a)" {3}, none
+
+//
+// Test the new driver when not bundling
+//
+// RUN: %clang -### --target=x86_64-linux-gnu --offload-new-driver -ccc-print-phases \
+// RUN:        --offload-device-only --offload-arch=amdgcnspirv,gfx1030 %s 2>&1 \
+// RUN: | FileCheck -check-prefix=SPIRV-ONLY %s
+//      SPIRV-ONLY: 0: input, "[[INPUT:.+]]", hip, (device-hip, gfx1030)
+// SPIRV-ONLY-NEXT: 1: preprocessor, {0}, hip-cpp-output, (device-hip, gfx1030)
+// SPIRV-ONLY-NEXT: 2: compiler, {1}, ir, (device-hip, gfx1030)
+// SPIRV-ONLY-NEXT: 3: backend, {2}, assembler, (device-hip, gfx1030)
+// SPIRV-ONLY-NEXT: 4: assembler, {3}, object, (device-hip, gfx1030)
+// SPIRV-ONLY-NEXT: 5: linker, {4}, image, (device-hip, gfx1030)
+// SPIRV-ONLY-NEXT: 6: offload, "device-hip (amdgcn-amd-amdhsa:gfx1030)" {5}, image
+// SPIRV-ONLY-NEXT: 7: input, "[[INPUT]]", hip, (device-hip, amdgcnspirv)
+// SPIRV-ONLY-NEXT: 8: preprocessor, {7}, hip-cpp-output, (device-hip, amdgcnspirv)
+// SPIRV-ONLY-NEXT: 9: compiler, {8}, ir, (device-hip, amdgcnspirv)
+// SPIRV-ONLY-NEXT: 10: backend, {9}, assembler, (device-hip, amdgcnspirv)
+// SPIRV-ONLY-NEXT: 11: assembler, {10}, object, (device-hip, amdgcnspirv)
+// SPIRV-ONLY-NEXT: 12: linker, {11}, image, (device-hip, amdgcnspirv)
+// SPIRV-ONLY-NEXT: 13: offload, "device-hip (spirv64-amd-amdhsa:amdgcnspirv)" {12}, image
+// SPIRV-ONLY-NEXT: 14: linker, {6, 13}, hip-fatbin, (device-hip)
+// SPIRV-ONLY-NEXT: 15: offload, "device-hip (amdgcn-amd-amdhsa)" {14}, none

[sarnex]

me yelling into the clouds: maybe one day we can have the triple functions actually make sense for GPU targets

[sarnex]

it looks like getSingleOffloadToolChain is just doing getOffloadToolChains<Action::OFK_HIP>().first->second, do we get some benefit from explicitly doing it here? is it to get around the asserts?

[jhuber6]

Yes, it avoids the asserts. It assumes that there's only one, but in this case we just need either one to run it's HIP linking job.

[sarnex]

got it, thanks

[arsenm]

demorgan condition and/or extract this into some kind of named predicate with a comment