[PATCH] D141705: [HLSL] [Dirver] add dxv as a Driver Action Job

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:4216
+  llvm::Triple T(getTargetTriple());
+  if (T.getArch() == llvm::Triple::dxil && !Args.getLastArg(options::OPT_dxc_disable_validation)) {
+    // Only add action when 'dxv' exists.
----------------
Could we move this logic into the HLSL ToolChain like we do with CUDA / ROCm? You should be able to then cast the toolchain to `HLSLToolChain` and query it here.


================
Comment at: clang/lib/Driver/Driver.cpp:4217
+  if (T.getArch() == llvm::Triple::dxil && !Args.getLastArg(options::OPT_dxc_disable_validation)) {
+    // Only add action when 'dxv' exists.
+    std::string DxvPath = GetProgramPath("dxv", C.getDefaultToolChain());
----------------



================
Comment at: clang/lib/Driver/Driver.cpp:4222
+      Actions.push_back(
+          C.MakeAction<BinaryAnalyzeJobAction>(LastAction, types::TY_Nothing));
+    } else {
----------------
Why is the type `TY_Nothing`? The `ConstructJob` invocation shows it using `-o`. So it should have output?


================
Comment at: clang/lib/Driver/Driver.cpp:4224
+    } else {
+      // Not find dxv.
+      C.getDriver().Diag(diag::warn_drv_dxc_missing_dxv);
----------------



================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:171
+    // discover the dxv executable.
+    getProgramPaths().push_back(getDriver().Dir);
+}
----------------
Just to check since I'm not really familiar at all with this toolchain, but does `dxv` exist as a clang tool? This path exists to search in the `/path/to/llvm/install/bin` directory. If it's an external binary this shouldn't be necessary.


================
Comment at: clang/test/Driver/dxc_dxv_path.hlsl:11
+// VD-NOT:dxv not found
+// VD:"-triple" "dxil-unknown-shadermodel6.3-library"
+
----------------
Lines that check clang typically start with `"-cc1"`


================
Comment at: clang/test/Driver/dxc_dxv_path.hlsl:14-15
+// RUN: %clang_dxc -Tlib_6_3 -ccc-print-bindings --dxv-path=%T -Fo %t.dxc  %s 2>&1 | FileCheck %s --check-prefix=BINDINGS
+// BINDINGS: "dxil-unknown-shadermodel6.3-library" - "clang", inputs: ["{{.*}}dxc_dxv_path.hlsl"], output: "[[DXC:.+]].dxc"
+// BINDINGS: "dxil-unknown-shadermodel6.3-library" - "hlsl::Validator", inputs: ["[[DXC]].dxc"], output: (nothing)
----------------
Can we get a test for `-ccc-print-phases` as well? Also, I wouldn't bother checking the test file's name and it's good practice to stipulate `NEXT` on these kinds of tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141705/new/

https://reviews.llvm.org/D141705