https://github.com/llvm-beanz created
https://github.com/llvm/llvm-project/pull/130436
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.
>From 0055f432469fa0eedf333e1a2d50025d822301cd Mon Sep 17 00:00:00 2001
From: Chris Bieneman <chris.biene...@me.com>
Date: Sat, 8 Mar 2025 14:20:07 -0600
Subject: [PATCH] [HLSL][Driver] Use temporary files correctly
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.
---
clang/lib/Driver/Driver.cpp | 34 +++++++++++++++++----
clang/lib/Driver/ToolChains/HLSL.cpp | 26 +++++++++++++---
clang/lib/Driver/ToolChains/HLSL.h | 2 ++
clang/test/Driver/HLSL/metal-converter.hlsl | 11 +++++--
clang/test/Driver/dxc_dxv_path.hlsl | 6 ++--
5 files changed, 62 insertions(+), 17 deletions(-)
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
