[HLSL][Driver] Use temporary files correctly

[llvm-beanz]

This updates the DXV and Metal Converter actions to properly use temporary files created by the driver. I've abstracted away a check to determine if an action is the last in the sequence because we may have between 1 and 3 actions depending on the arguments and environment.

[llvmbot]

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Chris B (llvm-beanz)

Changes

This updates the DXV and Metal Converter actions to properly use temporary files created by the driver. I've abstracted away a check to determine if an action is the last in the sequence because we may have between 1 and 3 actions depending on the arguments and environment.

---

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

5 Files Affected:

• (modified) clang/lib/Driver/Driver.cpp (+28-6)
• (modified) clang/lib/Driver/ToolChains/HLSL.cpp (+21-5)
• (modified) clang/lib/Driver/ToolChains/HLSL.h (+2)
• (modified) clang/test/Driver/HLSL/metal-converter.hlsl (+8-3)
• (modified) clang/test/Driver/dxc_dxv_path.hlsl (+3-3)

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 08ae8173db6df..9457a19255f21 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -4669,7 +4669,7 @@ void Driver::BuildActions(Compilation &C, DerivedArgList &Args,
       Actions.push_back(C.MakeAction<BinaryAnalyzeJobAction>(
           LastAction, types::TY_DX_CONTAINER));
     }
-    if (Args.getLastArg(options::OPT_metal)) {
+    if (TC.requiresBinaryTranslation(Args)) {
       Action *LastAction = Actions.back();
       // Metal shader converter runs on DXIL containers, which can either be
       // validated (in which case they are TY_DX_CONTAINER), or unvalidated
@@ -5219,8 +5219,14 @@ void Driver::BuildJobs(Compilation &C) const {
     unsigned NumOutputs = 0;
     unsigned NumIfsOutputs = 0;
     for (const Action *A : C.getActions()) {
+      // The actions below do not increase the number of outputs, when operating
+      // on DX containers.
+      if (A->getType() == types::TY_DX_CONTAINER &&
+          (A->getKind() == clang::driver::Action::BinaryAnalyzeJobClass ||
+           A->getKind() == clang::driver::Action::BinaryTranslatorJobClass))
+        continue;
+
       if (A->getType() != types::TY_Nothing &&
-          A->getType() != types::TY_DX_CONTAINER &&
           !(A->getKind() == Action::IfsMergeJobClass ||
             (A->getType() == clang::driver::types::TY_IFS_CPP &&
              A->getKind() == clang::driver::Action::CompileJobClass &&
@@ -6221,11 +6227,27 @@ const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
     return C.addResultFile(C.getArgs().MakeArgString(FcValue.str()), &JA);
   }
 
-  if (JA.getType() == types::TY_Object &&
-      C.getArgs().hasArg(options::OPT_dxc_Fo)) {
+  if ((JA.getType() == types::TY_Object &&
+       C.getArgs().hasArg(options::OPT_dxc_Fo)) ||
+      JA.getType() == types::TY_DX_CONTAINER) {
     StringRef FoValue = C.getArgs().getLastArgValue(options::OPT_dxc_Fo);
-    // TODO: Should we use `MakeCLOutputFilename` here? If so, we can probably
-    // handle this as part of the SLASH_Fo handling below.
+    // If we are targeting DXIL and not validating or translating, we should set
+    // the final result file. Otherwise we should emit to a temporary.
+    if (C.getDefaultToolChain().getTriple().isDXIL()) {
+      const auto &TC = static_cast<const toolchains::HLSLToolChain &>(
+          C.getDefaultToolChain());
+      // Fo can be empty here if the validator is running for a compiler flow
+      // that is using Fc or just printing disassembly.
+      if (TC.isLastJob(C.getArgs(), JA.getKind()) && !FoValue.empty())
+        return C.addResultFile(C.getArgs().MakeArgString(FoValue.str()), &JA);
+      StringRef Name = llvm::sys::path::filename(BaseInput);
+      std::pair<StringRef, StringRef> Split = Name.split('.');
+      const char *Suffix = types::getTypeTempSuffix(JA.getType(), true);
+      return CreateTempFile(C, Split.first, Suffix, false);
+    }
+    // We don't have SPIRV-val integrated (yet), so for now we can assume this
+    // is the final output.
+    assert(C.getDefaultToolChain().getTriple().isSPIRV());
     return C.addResultFile(C.getArgs().MakeArgString(FoValue.str()), &JA);
   }
 
diff --git a/clang/lib/Driver/ToolChains/HLSL.cpp b/clang/lib/Driver/ToolChains/HLSL.cpp
index 62e4d14390b90..22498bff1f251 100644
--- a/clang/lib/Driver/ToolChains/HLSL.cpp
+++ b/clang/lib/Driver/ToolChains/HLSL.cpp
@@ -186,12 +186,9 @@ void tools::hlsl::Validator::ConstructJob(Compilation &C, const JobAction &JA,
   ArgStringList CmdArgs;
   assert(Inputs.size() == 1 && "Unable to handle multiple inputs.");
   const InputInfo &Input = Inputs[0];
-  assert(Input.isFilename() && "Unexpected verify input");
-  // Grabbing the output of the earlier cc1 run.
   CmdArgs.push_back(Input.getFilename());
-  // Use the same name as output.
   CmdArgs.push_back("-o");
-  CmdArgs.push_back(Input.getFilename());
+  CmdArgs.push_back(Output.getFilename());
 
   const char *Exec = Args.MakeArgString(DxvPath);
   C.addCommand(std::make_unique<Command>(JA, *this, ResponseFileSupport::None(),
@@ -204,10 +201,11 @@ void tools::hlsl::MetalConverter::ConstructJob(
     const char *LinkingOutput) const {
   std::string MSCPath = getToolChain().GetProgramPath("metal-shaderconverter");
   ArgStringList CmdArgs;
+  assert(Inputs.size() == 1 && "Unable to handle multiple inputs.");
   const InputInfo &Input = Inputs[0];
   CmdArgs.push_back(Input.getFilename());
   CmdArgs.push_back("-o");
-  CmdArgs.push_back(Input.getFilename());
+  CmdArgs.push_back(Output.getFilename());
 
   const char *Exec = Args.MakeArgString(MSCPath);
   C.addCommand(std::make_unique<Command>(JA, *this, ResponseFileSupport::None(),
@@ -321,3 +319,21 @@ bool HLSLToolChain::requiresValidation(DerivedArgList &Args) const {
   getDriver().Diag(diag::warn_drv_dxc_missing_dxv);
   return false;
 }
+
+bool HLSLToolChain::requiresBinaryTranslation(DerivedArgList &Args) const {
+  return Args.hasArg(options::OPT_metal) && Args.hasArg(options::OPT_dxc_Fo);
+}
+
+bool HLSLToolChain::isLastJob(DerivedArgList &Args,
+                              Action::ActionClass AC) const {
+  bool HasTranslation = requiresBinaryTranslation(Args);
+  bool HasValidation = requiresValidation(Args);
+  // If translation and validation are not required, we should only have one
+  // action.
+  if (!HasTranslation && !HasValidation)
+    return true;
+  if ((HasTranslation && AC == Action::BinaryTranslatorJobClass) ||
+      (!HasTranslation && HasValidation && AC == Action::BinaryAnalyzeJobClass))
+    return true;
+  return false;
+}
diff --git a/clang/lib/Driver/ToolChains/HLSL.h b/clang/lib/Driver/ToolChains/HLSL.h
index 86dd65f0b80c6..3824b4252324b 100644
--- a/clang/lib/Driver/ToolChains/HLSL.h
+++ b/clang/lib/Driver/ToolChains/HLSL.h
@@ -64,6 +64,8 @@ class LLVM_LIBRARY_VISIBILITY HLSLToolChain : public ToolChain {
                 Action::OffloadKind DeviceOffloadKind) const override;
   static std::optional<std::string> parseTargetProfile(StringRef TargetProfile);
   bool requiresValidation(llvm::opt::DerivedArgList &Args) const;
+  bool requiresBinaryTranslation(llvm::opt::DerivedArgList &Args) const;
+  bool isLastJob(llvm::opt::DerivedArgList &Args, Action::ActionClass AC) const;
 
   // Set default DWARF version to 4 for DXIL uses version 4.
   unsigned GetDefaultDwarfVersion() const override { return 4; }
diff --git a/clang/test/Driver/HLSL/metal-converter.hlsl b/clang/test/Driver/HLSL/metal-converter.hlsl
index 4402e5044dc7b..536f24be6e73b 100644
--- a/clang/test/Driver/HLSL/metal-converter.hlsl
+++ b/clang/test/Driver/HLSL/metal-converter.hlsl
@@ -1,10 +1,15 @@
-// RUN: %clang_dxc -T cs_6_0 %s -metal -Fo tmp.mtl -### 2>&1 | FileCheck %s
-// RUN: %clang_dxc -T cs_6_0 %s -metal -Vd -Fo tmp.mtl -### 2>&1 | FileCheck %s
-// CHECK: "{{.*}}metal-shaderconverter{{(.exe)?}}" "tmp.mtl" "-o" "tmp.mtl"
+// RUN: %clang_dxc -T cs_6_0 %s -metal -Fo %t.mtl -### 2>&1 | FileCheck %s
+// RUN: %clang_dxc -T cs_6_0 %s -metal -Vd -Fo %t.mtl -### 2>&1 | FileCheck %s
+// CHECK: "{{.*}}metal-shaderconverter{{(.exe)?}}" "{{.*}}.obj" "-o" "{{.*}}.mtl"
 
 // RUN: %clang_dxc -T cs_6_0 %s -metal -### 2>&1 | FileCheck --check-prefix=NO_MTL %s
 // NO_MTL-NOT: metal-shaderconverter
 
+// RUN: echo "dxv" > %T/dxv && chmod 754 %T/dxv
+// RUN: %clang_dxc -T cs_6_0 %s --dxv-path=%T -metal -Fo %t.mtl -### 2>&1 | FileCheck --check-prefix=DXV %s
+// DXV: "{{.*}}dxv{{(.exe)?}}" "{{.*}}.obj" "-o" "{{.*}}.dxo"
+// DXV: "{{.*}}metal-shaderconverter{{(.exe)?}}" "{{.*}}.dxo" "-o" "{{.*}}.mtl"
+
 RWBuffer<float4> In : register(u0, space0);
 RWBuffer<float4> Out : register(u1, space4);
 
diff --git a/clang/test/Driver/dxc_dxv_path.hlsl b/clang/test/Driver/dxc_dxv_path.hlsl
index db2c87063ac31..55a07f34a648e 100644
--- a/clang/test/Driver/dxc_dxv_path.hlsl
+++ b/clang/test/Driver/dxc_dxv_path.hlsl
@@ -4,15 +4,15 @@
 // CHECK:dxv not found
 
 // RUN: echo "dxv" > %T/dxv && chmod 754 %T/dxv && %clang_dxc --dxv-path=%T %s -Tlib_6_3 -### 2>&1 | FileCheck %s --check-prefix=DXV_PATH
-// DXV_PATH:dxv{{(.exe)?}}" "-" "-o" "-"
+// DXV_PATH:dxv{{(.exe)?}}" "-" "-o" "{{.*}}.dxo"
 
 // RUN: %clang_dxc -I test -Vd -Tlib_6_3  -### %s 2>&1 | FileCheck %s --check-prefix=VD
 // VD:"-cc1"{{.*}}"-triple" "dxilv1.3-unknown-shadermodel6.3-library"
 // VD-NOT:dxv not found
 
 // RUN: %clang_dxc -Tlib_6_3 -ccc-print-bindings --dxv-path=%T -Fo %t.dxo  %s 2>&1 | FileCheck %s --check-prefix=BINDINGS
-// BINDINGS: "dxilv1.3-unknown-shadermodel6.3-library" - "clang", inputs: ["[[INPUT:.+]]"], output: "[[DXC:.+]].dxo"
-// BINDINGS-NEXT: "dxilv1.3-unknown-shadermodel6.3-library" - "hlsl::Validator", inputs: ["[[DXC]].dxo"]
+// BINDINGS: "dxilv1.3-unknown-shadermodel6.3-library" - "clang", inputs: ["[[INPUT:.+]]"], output: "[[obj:.+]].obj"
+// BINDINGS-NEXT: "dxilv1.3-unknown-shadermodel6.3-library" - "hlsl::Validator", inputs: ["[[obj]].obj"], output: "{{.+}}.dxo"
 
 // RUN: %clang_dxc -Tlib_6_3 -ccc-print-phases --dxv-path=%T -Fo %t.dxc  %s 2>&1 | FileCheck %s --check-prefix=PHASES