Skip to content

[DXIL] Set DXIL Version in DXIL target triple based on shader model version #91407

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

Conversation

bharadwajy
Copy link
Contributor

@bharadwajy bharadwajy commented May 7, 2024

This change set restores the PR #90809 that was reverted to address ASAN failures and includes a fix for the ASAN failures. The fix to address ASAN failure is in commit 8522e36 of this PR.

Following is the description of the change:

An earlier commit provided a way to decouple DXIL version from Shader Model version
by representing the DXIL version as SubArch in the DXIL Target Triple and adding
corresponding valid DXIL Arch types.

This change constructs DXIL target triple with DXIL version that is deduced
from Shader Model version specified in the following scenarios:

  1. When compilation target profile is specified:
    For e.g., DXIL target triple dxilv1.8-unknown-shader6.8-library is constructed
    when -T lib_6_8 is specified.
  2. When DXIL target triple without DXIL version is specified:
    For e.g., DXIL target triple dxilv1.8-pc-shadermodel6.8-library is constructed
    when -mtriple=dxil-pc-shadermodel6.8-library is specified.

Note that this is an alternate / expanded implementation to that proposed in PR #89823.

PR #89823 implements functionality (1) above. However, it requires any DXIL target
triple to explicitly include DXIL version anywhere DXIL target triple is expected (e.g.,
dxilv1.8-pc-shadermodel6.8-library) while a triple without DXIL version
(e.g., dxil-pc-shadermodel6.8-library) is considered invalid. This functionality is
implemented as a change to tryParseProfile() and is included in this PR.

This PR adds the functionality (2) to eliminate the above requirement to explicitly
specify DXIL version in the target triple thereby considering a triple without DXIL
version ( e.g., dxil-pc-shadermodel6.8-library) to be valid. A triple with DXIL version
is inferred based on the shader mode version specified. If no shader model version is
specified, DXIL version is defaulted to 1.0. This functionality is implemented in the
"Special case logic ..." section of Triple::normalize().
 
Updated relevant HLSL tests that check for target triple.

Validated that Clang (check-clang), LLVM (check-llvm) regression tests and
TargetParserTests built with sanitizer, pass.

bharadwajy added 2 commits May 7, 2024 16:39
… on shader model version" (llvm#91290)"

This restores commit 178ff39.

An earlier commit provided a way to decouple DXIL version from Shader
Model version by representing the DXIL version as `SubArch` in the DXIL
Target Triple and adding corresponding valid DXIL Arch types.

This change constructs DXIL target triple with DXIL version that is
deduced from Shader Model version specified in the following scenarios:

 1. When compilation target profile is specified:
    For e.g., DXIL target triple `dxilv1.8-unknown-shader6.8-library` is
    constructed when `-T lib_6_8` is specified.
 2. When DXIL target triple without DXIL version is specified:
    For e.g., DXIL target triple `dxilv1.8-pc-shadermodel6.8-library` is
    constructed when `-mtriple=dxil-pc-shadermodel6.8-library` is specified.

    Updated relevant HLSL tests that check for target triple.
@bharadwajy bharadwajy requested a review from bogner May 7, 2024 22:13
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support llvm:ir labels May 7, 2024
@bharadwajy bharadwajy requested a review from llvm-beanz May 7, 2024 22:13
@llvmbot
Copy link
Member

llvmbot commented May 7, 2024

@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-clang-driver

Author: S. Bharadwaj Yadavalli (bharadwajy)

Changes

This change set restores the PR #90809 that was reverted to address ASAN failures and includes a fix for the ASAN failures.

Following is the description of the change:

An earlier commit provided a way to decouple DXIL version from Shader Model version
by representing the DXIL version as SubArch in the DXIL Target Triple and adding
corresponding valid DXIL Arch types.

This change constructs DXIL target triple with DXIL version that is deduced
from Shader Model version specified in the following scenarios:

  1. When compilation target profile is specified:
    For e.g., DXIL target triple dxilv1.8-unknown-shader6.8-library is constructed
    when -T lib_6_8 is specified.
  2. When DXIL target triple without DXIL version is specified:
    For e.g., DXIL target triple dxilv1.8-pc-shadermodel6.8-library is constructed
    when -mtriple=dxil-pc-shadermodel6.8-library is specified.

Note that this is an alternate / expanded implementation to that proposed in PR #89823.

PR #89823 implements functionality (1) above. However, it requires any DXIL target
triple to explicitly include DXIL version anywhere DXIL target triple is expected (e.g.,
dxilv1.8-pc-shadermodel6.8-library) while a triple without DXIL version
(e.g., dxil-pc-shadermodel6.8-library) is considered invalid. This functionality is
implemented as a change to tryParseProfile() and is included in this PR.

This PR adds the functionality (2) to eliminate the above requirement to explicitly
specify DXIL version in the target triple thereby considering a triple without DXIL
version ( e.g., dxil-pc-shadermodel6.8-library) to be valid. A triple with DXIL version
is inferred based on the shader mode version specified. If no shader model version is
specified, DXIL version is defaulted to 1.0. This functionality is implemented in the
"Special case logic ..." section of Triple::normalize().
 
Updated relevant HLSL tests that check for target triple.

Validated that Clang (check-clang), LLVM (check-llvm) regression tests and
TargetParserTests built with sanitizer, pass.


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

10 Files Affected:

  • (modified) clang/lib/Basic/Targets.cpp (+1-1)
  • (modified) clang/lib/Driver/ToolChains/HLSL.cpp (+42-2)
  • (modified) clang/test/CodeGenHLSL/basic-target.c (+1-1)
  • (modified) clang/test/Driver/dxc_dxv_path.hlsl (+3-3)
  • (modified) clang/test/Options/enable_16bit_types_validation.hlsl (+2-2)
  • (modified) clang/unittests/Driver/DXCModeTest.cpp (+12-10)
  • (modified) llvm/include/llvm/TargetParser/Triple.h (+1)
  • (modified) llvm/lib/IR/Verifier.cpp (+2-2)
  • (modified) llvm/lib/TargetParser/Triple.cpp (+86)
  • (modified) llvm/unittests/TargetParser/TripleTest.cpp (+16)
diff --git a/clang/lib/Basic/Targets.cpp b/clang/lib/Basic/Targets.cpp
index e3283510c6aa..dc1792b3471e 100644
--- a/clang/lib/Basic/Targets.cpp
+++ b/clang/lib/Basic/Targets.cpp
@@ -760,7 +760,7 @@ using namespace clang::targets;
 TargetInfo *
 TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
                              const std::shared_ptr<TargetOptions> &Opts) {
-  llvm::Triple Triple(Opts->Triple);
+  llvm::Triple Triple(llvm::Triple::normalize(Opts->Triple));
 
   // Construct the target
   std::unique_ptr<TargetInfo> Target = AllocateTarget(Triple, *Opts);
diff --git a/clang/lib/Driver/ToolChains/HLSL.cpp b/clang/lib/Driver/ToolChains/HLSL.cpp
index 558e4db46f81..8286e3be2180 100644
--- a/clang/lib/Driver/ToolChains/HLSL.cpp
+++ b/clang/lib/Driver/ToolChains/HLSL.cpp
@@ -98,9 +98,49 @@ std::optional<std::string> tryParseProfile(StringRef Profile) {
   else if (llvm::getAsUnsignedInteger(Parts[2], 0, Minor))
     return std::nullopt;
 
-  // dxil-unknown-shadermodel-hull
+  // Determine DXIL version using the minor version number of Shader
+  // Model version specified in target profile. Prior to decoupling DXIL version
+  // numbering from that of Shader Model DXIL version 1.Y corresponds to SM 6.Y.
+  // E.g., dxilv1.Y-unknown-shadermodelX.Y-hull
   llvm::Triple T;
-  T.setArch(Triple::ArchType::dxil);
+  Triple::SubArchType SubArch = llvm::Triple::NoSubArch;
+  switch (Minor) {
+  case 0:
+    SubArch = llvm::Triple::DXILSubArch_v1_0;
+    break;
+  case 1:
+    SubArch = llvm::Triple::DXILSubArch_v1_1;
+    break;
+  case 2:
+    SubArch = llvm::Triple::DXILSubArch_v1_2;
+    break;
+  case 3:
+    SubArch = llvm::Triple::DXILSubArch_v1_3;
+    break;
+  case 4:
+    SubArch = llvm::Triple::DXILSubArch_v1_4;
+    break;
+  case 5:
+    SubArch = llvm::Triple::DXILSubArch_v1_5;
+    break;
+  case 6:
+    SubArch = llvm::Triple::DXILSubArch_v1_6;
+    break;
+  case 7:
+    SubArch = llvm::Triple::DXILSubArch_v1_7;
+    break;
+  case 8:
+    SubArch = llvm::Triple::DXILSubArch_v1_8;
+    break;
+  case OfflineLibMinor:
+    // Always consider minor version x as the latest supported DXIL version
+    SubArch = llvm::Triple::LatestDXILSubArch;
+    break;
+  default:
+    // No DXIL Version corresponding to specified Shader Model version found
+    return std::nullopt;
+  }
+  T.setArch(Triple::ArchType::dxil, SubArch);
   T.setOSName(Triple::getOSTypeName(Triple::OSType::ShaderModel).str() +
               VersionTuple(Major, Minor).getAsString());
   T.setEnvironment(Kind);
diff --git a/clang/test/CodeGenHLSL/basic-target.c b/clang/test/CodeGenHLSL/basic-target.c
index 8db711c3f2a5..b97ebf90a7a1 100644
--- a/clang/test/CodeGenHLSL/basic-target.c
+++ b/clang/test/CodeGenHLSL/basic-target.c
@@ -7,4 +7,4 @@
 // RUN: %clang -target dxil-pc-shadermodel6.0-geometry -S -emit-llvm -o - %s | FileCheck %s
 
 // CHECK: target datalayout = "e-m:e-p:32:32-i1:32-i8:8-i16:16-i32:32-i64:64-f16:16-f32:32-f64:64-n8:16:32:64"
-// CHECK: target triple = "dxil-pc-shadermodel6.0-{{[a-z]+}}"
+// CHECK: target triple = "dxilv1.0-pc-shadermodel6.0-{{[a-z]+}}"
diff --git a/clang/test/Driver/dxc_dxv_path.hlsl b/clang/test/Driver/dxc_dxv_path.hlsl
index 3d8e90d0d919..4845de11d5b0 100644
--- a/clang/test/Driver/dxc_dxv_path.hlsl
+++ b/clang/test/Driver/dxc_dxv_path.hlsl
@@ -7,12 +7,12 @@
 // DXV_PATH:dxv{{(.exe)?}}" "-" "-o" "-"
 
 // RUN: %clang_dxc -I test -Vd -Tlib_6_3  -### %s 2>&1 | FileCheck %s --check-prefix=VD
-// VD:"-cc1"{{.*}}"-triple" "dxil-unknown-shadermodel6.3-library"
+// 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: "dxil-unknown-shadermodel6.3-library" - "clang", inputs: ["[[INPUT:.+]]"], output: "[[DXC:.+]].dxo"
-// BINDINGS-NEXT: "dxil-unknown-shadermodel6.3-library" - "hlsl::Validator", inputs: ["[[DXC]].dxo"]
+// 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"]
 
 // RUN: %clang_dxc -Tlib_6_3 -ccc-print-phases --dxv-path=%T -Fo %t.dxc  %s 2>&1 | FileCheck %s --check-prefix=PHASES
 
diff --git a/clang/test/Options/enable_16bit_types_validation.hlsl b/clang/test/Options/enable_16bit_types_validation.hlsl
index 89fe26790c52..bcb217e8982e 100644
--- a/clang/test/Options/enable_16bit_types_validation.hlsl
+++ b/clang/test/Options/enable_16bit_types_validation.hlsl
@@ -9,11 +9,11 @@
 // HV_invalid_2017: error: '-enable-16bit-types' option requires target HLSL Version >= 2018 and shader model >= 6.2, but HLSL Version is 'hlsl2017' and shader model is '6.4'
 // TP_invalid: error: '-enable-16bit-types' option requires target HLSL Version >= 2018 and shader model >= 6.2, but HLSL Version is 'hlsl2021' and shader model is '6.0'
 
-// valid_2021: "dxil-unknown-shadermodel6.4-library"
+// valid_2021: "dxilv1.4-unknown-shadermodel6.4-library"
 // valid_2021-SAME: "-std=hlsl2021"
 // valid_2021-SAME: "-fnative-half-type"
 
-// valid_2018: "dxil-unknown-shadermodel6.4-library"
+// valid_2018: "dxilv1.4-unknown-shadermodel6.4-library"
 // valid_2018-SAME: "-std=hlsl2018"
 // valid_2018-SAME: "-fnative-half-type"
 
diff --git a/clang/unittests/Driver/DXCModeTest.cpp b/clang/unittests/Driver/DXCModeTest.cpp
index b3767c042edb..416723d498a2 100644
--- a/clang/unittests/Driver/DXCModeTest.cpp
+++ b/clang/unittests/Driver/DXCModeTest.cpp
@@ -68,25 +68,27 @@ TEST(DxcModeTest, TargetProfileValidation) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer);
 
-  validateTargetProfile("-Tvs_6_0", "dxil--shadermodel6.0-vertex",
+  validateTargetProfile("-Tvs_6_0", "dxilv1.0--shadermodel6.0-vertex",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Ths_6_1", "dxil--shadermodel6.1-hull",
+  validateTargetProfile("-Ths_6_1", "dxilv1.1--shadermodel6.1-hull",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tds_6_2", "dxil--shadermodel6.2-domain",
+  validateTargetProfile("-Tds_6_2", "dxilv1.2--shadermodel6.2-domain",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tds_6_2", "dxil--shadermodel6.2-domain",
+  validateTargetProfile("-Tds_6_2", "dxilv1.2--shadermodel6.2-domain",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tgs_6_3", "dxil--shadermodel6.3-geometry",
+  validateTargetProfile("-Tgs_6_3", "dxilv1.3--shadermodel6.3-geometry",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tps_6_4", "dxil--shadermodel6.4-pixel",
+  validateTargetProfile("-Tps_6_4", "dxilv1.4--shadermodel6.4-pixel",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tcs_6_5", "dxil--shadermodel6.5-compute",
+  validateTargetProfile("-Tcs_6_5", "dxilv1.5--shadermodel6.5-compute",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tms_6_6", "dxil--shadermodel6.6-mesh",
+  validateTargetProfile("-Tms_6_6", "dxilv1.6--shadermodel6.6-mesh",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tas_6_7", "dxil--shadermodel6.7-amplification",
+  validateTargetProfile("-Tas_6_7", "dxilv1.7--shadermodel6.7-amplification",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tlib_6_x", "dxil--shadermodel6.15-library",
+  validateTargetProfile("-Tcs_6_8", "dxilv1.8--shadermodel6.8-compute",
+                        InMemoryFileSystem, Diags);
+  validateTargetProfile("-Tlib_6_x", "dxilv1.8--shadermodel6.15-library",
                         InMemoryFileSystem, Diags);
 
   // Invalid tests.
diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 7da30e6cf96f..4e357cddcf2a 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -176,6 +176,7 @@ class Triple {
     DXILSubArch_v1_6,
     DXILSubArch_v1_7,
     DXILSubArch_v1_8,
+    LatestDXILSubArch = DXILSubArch_v1_8,
   };
   enum VendorType {
     UnknownVendor,
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index aa8160d18edd..50f8d6ec8420 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -152,8 +152,8 @@ struct VerifierSupport {
   bool TreatBrokenDebugInfoAsError = true;
 
   explicit VerifierSupport(raw_ostream *OS, const Module &M)
-      : OS(OS), M(M), MST(&M), TT(M.getTargetTriple()), DL(M.getDataLayout()),
-        Context(M.getContext()) {}
+      : OS(OS), M(M), MST(&M), TT(Triple::normalize(M.getTargetTriple())),
+        DL(M.getDataLayout()), Context(M.getContext()) {}
 
 private:
   void Write(const Module *M) {
diff --git a/llvm/lib/TargetParser/Triple.cpp b/llvm/lib/TargetParser/Triple.cpp
index f3f244c814e7..35686c1ea900 100644
--- a/llvm/lib/TargetParser/Triple.cpp
+++ b/llvm/lib/TargetParser/Triple.cpp
@@ -115,6 +115,31 @@ StringRef Triple::getArchName(ArchType Kind, SubArchType SubArch) {
     if (SubArch == AArch64SubArch_arm64e)
       return "arm64e";
     break;
+  case Triple::dxil:
+    switch (SubArch) {
+    case Triple::NoSubArch:
+    case Triple::DXILSubArch_v1_0:
+      return "dxilv1.0";
+    case Triple::DXILSubArch_v1_1:
+      return "dxilv1.1";
+    case Triple::DXILSubArch_v1_2:
+      return "dxilv1.2";
+    case Triple::DXILSubArch_v1_3:
+      return "dxilv1.3";
+    case Triple::DXILSubArch_v1_4:
+      return "dxilv1.4";
+    case Triple::DXILSubArch_v1_5:
+      return "dxilv1.5";
+    case Triple::DXILSubArch_v1_6:
+      return "dxilv1.6";
+    case Triple::DXILSubArch_v1_7:
+      return "dxilv1.7";
+    case Triple::DXILSubArch_v1_8:
+      return "dxilv1.8";
+    default:
+      break;
+    }
+    break;
   default:
     break;
   }
@@ -1014,6 +1039,53 @@ Triple::Triple(const Twine &ArchStr, const Twine &VendorStr, const Twine &OSStr,
     ObjectFormat = getDefaultFormat(*this);
 }
 
+static VersionTuple parseVersionFromName(StringRef Name);
+
+static StringRef getDXILArchNameFromShaderModel(StringRef ShaderModelStr) {
+  VersionTuple Ver =
+      parseVersionFromName(ShaderModelStr.drop_front(strlen("shadermodel")));
+  // Default DXIL minor version when Shader Model version is anything other
+  // than 6.[0...8] or 6.x (which translates to latest current SM version)
+  const unsigned SMMajor = 6;
+  if (!Ver.empty()) {
+    if (Ver.getMajor() == SMMajor) {
+      if (std::optional<unsigned> SMMinor = Ver.getMinor()) {
+        switch (*SMMinor) {
+        case 0:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_0);
+        case 1:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_1);
+        case 2:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_2);
+        case 3:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_3);
+        case 4:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_4);
+        case 5:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_5);
+        case 6:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_6);
+        case 7:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_7);
+        case 8:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_8);
+        default:
+          report_fatal_error("Unsupported Shader Model version", false);
+        }
+      }
+    }
+  } else {
+    // Special case: DXIL minor version is set to LatestCurrentDXILMinor for
+    // shadermodel6.x is
+    if (ShaderModelStr == "shadermodel6.x") {
+      return Triple::getArchName(Triple::dxil, Triple::LatestDXILSubArch);
+    }
+  }
+  // DXIL version corresponding to Shader Model version other than 6.Minor
+  // is 1.0
+  return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_0);
+}
+
 std::string Triple::normalize(StringRef Str) {
   bool IsMinGW32 = false;
   bool IsCygwin = false;
@@ -1206,6 +1278,20 @@ std::string Triple::normalize(StringRef Str) {
     }
   }
 
+  // Normalize DXIL triple if it does not include DXIL version number.
+  // Determine DXIL version number using the minor version number of Shader
+  // Model version specified in target triple, if any. Prior to decoupling DXIL
+  // version numbering from that of Shader Model DXIL version 1.Y corresponds to
+  // SM 6.Y. E.g., dxilv1.Y-unknown-shadermodelX.Y-hull
+  if (Components[0] == "dxil") {
+    if (Components.size() > 4) {
+      Components.resize(4);
+    }
+    // Add DXIL version only if shadermodel is specified in the triple
+    if (OS == Triple::ShaderModel) {
+      Components[0] = getDXILArchNameFromShaderModel(Components[2]);
+    }
+  }
   // Stick the corrected components back together to form the normalized string.
   return join(Components, "-");
 }
diff --git a/llvm/unittests/TargetParser/TripleTest.cpp b/llvm/unittests/TargetParser/TripleTest.cpp
index b8f5fbd87407..3112014d9efb 100644
--- a/llvm/unittests/TargetParser/TripleTest.cpp
+++ b/llvm/unittests/TargetParser/TripleTest.cpp
@@ -2454,4 +2454,20 @@ TEST(TripleTest, isArmMClass) {
     EXPECT_TRUE(T.isArmMClass());
   }
 }
+
+TEST(TripleTest, DXILNormaizeWithVersion) {
+  EXPECT_EQ("dxilv1.0-unknown-shadermodel6.0",
+            Triple::normalize("dxilv1.0--shadermodel6.0"));
+  EXPECT_EQ("dxilv1.0-unknown-shadermodel6.0",
+            Triple::normalize("dxil--shadermodel6.0"));
+  EXPECT_EQ("dxilv1.1-unknown-shadermodel6.1-library",
+            Triple::normalize("dxil-shadermodel6.1-unknown-library"));
+  EXPECT_EQ("dxilv1.8-unknown-shadermodel6.x-unknown",
+            Triple::normalize("dxil-unknown-shadermodel6.x-unknown"));
+  EXPECT_EQ("dxilv1.8-unknown-shadermodel6.x-unknown",
+            Triple::normalize("dxil-unknown-shadermodel6.x-unknown"));
+  EXPECT_EQ("dxil-unknown-unknown-unknown", Triple::normalize("dxil---"));
+  EXPECT_EQ("dxilv1.0-pc-shadermodel5.0-compute",
+            Triple::normalize("dxil-shadermodel5.0-pc-compute"));
+}
 } // end anonymous namespace

@llvmbot
Copy link
Member

llvmbot commented May 7, 2024

@llvm/pr-subscribers-clang

Author: S. Bharadwaj Yadavalli (bharadwajy)

Changes

This change set restores the PR #90809 that was reverted to address ASAN failures and includes a fix for the ASAN failures.

Following is the description of the change:

An earlier commit provided a way to decouple DXIL version from Shader Model version
by representing the DXIL version as SubArch in the DXIL Target Triple and adding
corresponding valid DXIL Arch types.

This change constructs DXIL target triple with DXIL version that is deduced
from Shader Model version specified in the following scenarios:

  1. When compilation target profile is specified:
    For e.g., DXIL target triple dxilv1.8-unknown-shader6.8-library is constructed
    when -T lib_6_8 is specified.
  2. When DXIL target triple without DXIL version is specified:
    For e.g., DXIL target triple dxilv1.8-pc-shadermodel6.8-library is constructed
    when -mtriple=dxil-pc-shadermodel6.8-library is specified.

Note that this is an alternate / expanded implementation to that proposed in PR #89823.

PR #89823 implements functionality (1) above. However, it requires any DXIL target
triple to explicitly include DXIL version anywhere DXIL target triple is expected (e.g.,
dxilv1.8-pc-shadermodel6.8-library) while a triple without DXIL version
(e.g., dxil-pc-shadermodel6.8-library) is considered invalid. This functionality is
implemented as a change to tryParseProfile() and is included in this PR.

This PR adds the functionality (2) to eliminate the above requirement to explicitly
specify DXIL version in the target triple thereby considering a triple without DXIL
version ( e.g., dxil-pc-shadermodel6.8-library) to be valid. A triple with DXIL version
is inferred based on the shader mode version specified. If no shader model version is
specified, DXIL version is defaulted to 1.0. This functionality is implemented in the
"Special case logic ..." section of Triple::normalize().
 
Updated relevant HLSL tests that check for target triple.

Validated that Clang (check-clang), LLVM (check-llvm) regression tests and
TargetParserTests built with sanitizer, pass.


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

10 Files Affected:

  • (modified) clang/lib/Basic/Targets.cpp (+1-1)
  • (modified) clang/lib/Driver/ToolChains/HLSL.cpp (+42-2)
  • (modified) clang/test/CodeGenHLSL/basic-target.c (+1-1)
  • (modified) clang/test/Driver/dxc_dxv_path.hlsl (+3-3)
  • (modified) clang/test/Options/enable_16bit_types_validation.hlsl (+2-2)
  • (modified) clang/unittests/Driver/DXCModeTest.cpp (+12-10)
  • (modified) llvm/include/llvm/TargetParser/Triple.h (+1)
  • (modified) llvm/lib/IR/Verifier.cpp (+2-2)
  • (modified) llvm/lib/TargetParser/Triple.cpp (+86)
  • (modified) llvm/unittests/TargetParser/TripleTest.cpp (+16)
diff --git a/clang/lib/Basic/Targets.cpp b/clang/lib/Basic/Targets.cpp
index e3283510c6aa..dc1792b3471e 100644
--- a/clang/lib/Basic/Targets.cpp
+++ b/clang/lib/Basic/Targets.cpp
@@ -760,7 +760,7 @@ using namespace clang::targets;
 TargetInfo *
 TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
                              const std::shared_ptr<TargetOptions> &Opts) {
-  llvm::Triple Triple(Opts->Triple);
+  llvm::Triple Triple(llvm::Triple::normalize(Opts->Triple));
 
   // Construct the target
   std::unique_ptr<TargetInfo> Target = AllocateTarget(Triple, *Opts);
diff --git a/clang/lib/Driver/ToolChains/HLSL.cpp b/clang/lib/Driver/ToolChains/HLSL.cpp
index 558e4db46f81..8286e3be2180 100644
--- a/clang/lib/Driver/ToolChains/HLSL.cpp
+++ b/clang/lib/Driver/ToolChains/HLSL.cpp
@@ -98,9 +98,49 @@ std::optional<std::string> tryParseProfile(StringRef Profile) {
   else if (llvm::getAsUnsignedInteger(Parts[2], 0, Minor))
     return std::nullopt;
 
-  // dxil-unknown-shadermodel-hull
+  // Determine DXIL version using the minor version number of Shader
+  // Model version specified in target profile. Prior to decoupling DXIL version
+  // numbering from that of Shader Model DXIL version 1.Y corresponds to SM 6.Y.
+  // E.g., dxilv1.Y-unknown-shadermodelX.Y-hull
   llvm::Triple T;
-  T.setArch(Triple::ArchType::dxil);
+  Triple::SubArchType SubArch = llvm::Triple::NoSubArch;
+  switch (Minor) {
+  case 0:
+    SubArch = llvm::Triple::DXILSubArch_v1_0;
+    break;
+  case 1:
+    SubArch = llvm::Triple::DXILSubArch_v1_1;
+    break;
+  case 2:
+    SubArch = llvm::Triple::DXILSubArch_v1_2;
+    break;
+  case 3:
+    SubArch = llvm::Triple::DXILSubArch_v1_3;
+    break;
+  case 4:
+    SubArch = llvm::Triple::DXILSubArch_v1_4;
+    break;
+  case 5:
+    SubArch = llvm::Triple::DXILSubArch_v1_5;
+    break;
+  case 6:
+    SubArch = llvm::Triple::DXILSubArch_v1_6;
+    break;
+  case 7:
+    SubArch = llvm::Triple::DXILSubArch_v1_7;
+    break;
+  case 8:
+    SubArch = llvm::Triple::DXILSubArch_v1_8;
+    break;
+  case OfflineLibMinor:
+    // Always consider minor version x as the latest supported DXIL version
+    SubArch = llvm::Triple::LatestDXILSubArch;
+    break;
+  default:
+    // No DXIL Version corresponding to specified Shader Model version found
+    return std::nullopt;
+  }
+  T.setArch(Triple::ArchType::dxil, SubArch);
   T.setOSName(Triple::getOSTypeName(Triple::OSType::ShaderModel).str() +
               VersionTuple(Major, Minor).getAsString());
   T.setEnvironment(Kind);
diff --git a/clang/test/CodeGenHLSL/basic-target.c b/clang/test/CodeGenHLSL/basic-target.c
index 8db711c3f2a5..b97ebf90a7a1 100644
--- a/clang/test/CodeGenHLSL/basic-target.c
+++ b/clang/test/CodeGenHLSL/basic-target.c
@@ -7,4 +7,4 @@
 // RUN: %clang -target dxil-pc-shadermodel6.0-geometry -S -emit-llvm -o - %s | FileCheck %s
 
 // CHECK: target datalayout = "e-m:e-p:32:32-i1:32-i8:8-i16:16-i32:32-i64:64-f16:16-f32:32-f64:64-n8:16:32:64"
-// CHECK: target triple = "dxil-pc-shadermodel6.0-{{[a-z]+}}"
+// CHECK: target triple = "dxilv1.0-pc-shadermodel6.0-{{[a-z]+}}"
diff --git a/clang/test/Driver/dxc_dxv_path.hlsl b/clang/test/Driver/dxc_dxv_path.hlsl
index 3d8e90d0d919..4845de11d5b0 100644
--- a/clang/test/Driver/dxc_dxv_path.hlsl
+++ b/clang/test/Driver/dxc_dxv_path.hlsl
@@ -7,12 +7,12 @@
 // DXV_PATH:dxv{{(.exe)?}}" "-" "-o" "-"
 
 // RUN: %clang_dxc -I test -Vd -Tlib_6_3  -### %s 2>&1 | FileCheck %s --check-prefix=VD
-// VD:"-cc1"{{.*}}"-triple" "dxil-unknown-shadermodel6.3-library"
+// 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: "dxil-unknown-shadermodel6.3-library" - "clang", inputs: ["[[INPUT:.+]]"], output: "[[DXC:.+]].dxo"
-// BINDINGS-NEXT: "dxil-unknown-shadermodel6.3-library" - "hlsl::Validator", inputs: ["[[DXC]].dxo"]
+// 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"]
 
 // RUN: %clang_dxc -Tlib_6_3 -ccc-print-phases --dxv-path=%T -Fo %t.dxc  %s 2>&1 | FileCheck %s --check-prefix=PHASES
 
diff --git a/clang/test/Options/enable_16bit_types_validation.hlsl b/clang/test/Options/enable_16bit_types_validation.hlsl
index 89fe26790c52..bcb217e8982e 100644
--- a/clang/test/Options/enable_16bit_types_validation.hlsl
+++ b/clang/test/Options/enable_16bit_types_validation.hlsl
@@ -9,11 +9,11 @@
 // HV_invalid_2017: error: '-enable-16bit-types' option requires target HLSL Version >= 2018 and shader model >= 6.2, but HLSL Version is 'hlsl2017' and shader model is '6.4'
 // TP_invalid: error: '-enable-16bit-types' option requires target HLSL Version >= 2018 and shader model >= 6.2, but HLSL Version is 'hlsl2021' and shader model is '6.0'
 
-// valid_2021: "dxil-unknown-shadermodel6.4-library"
+// valid_2021: "dxilv1.4-unknown-shadermodel6.4-library"
 // valid_2021-SAME: "-std=hlsl2021"
 // valid_2021-SAME: "-fnative-half-type"
 
-// valid_2018: "dxil-unknown-shadermodel6.4-library"
+// valid_2018: "dxilv1.4-unknown-shadermodel6.4-library"
 // valid_2018-SAME: "-std=hlsl2018"
 // valid_2018-SAME: "-fnative-half-type"
 
diff --git a/clang/unittests/Driver/DXCModeTest.cpp b/clang/unittests/Driver/DXCModeTest.cpp
index b3767c042edb..416723d498a2 100644
--- a/clang/unittests/Driver/DXCModeTest.cpp
+++ b/clang/unittests/Driver/DXCModeTest.cpp
@@ -68,25 +68,27 @@ TEST(DxcModeTest, TargetProfileValidation) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer);
 
-  validateTargetProfile("-Tvs_6_0", "dxil--shadermodel6.0-vertex",
+  validateTargetProfile("-Tvs_6_0", "dxilv1.0--shadermodel6.0-vertex",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Ths_6_1", "dxil--shadermodel6.1-hull",
+  validateTargetProfile("-Ths_6_1", "dxilv1.1--shadermodel6.1-hull",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tds_6_2", "dxil--shadermodel6.2-domain",
+  validateTargetProfile("-Tds_6_2", "dxilv1.2--shadermodel6.2-domain",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tds_6_2", "dxil--shadermodel6.2-domain",
+  validateTargetProfile("-Tds_6_2", "dxilv1.2--shadermodel6.2-domain",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tgs_6_3", "dxil--shadermodel6.3-geometry",
+  validateTargetProfile("-Tgs_6_3", "dxilv1.3--shadermodel6.3-geometry",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tps_6_4", "dxil--shadermodel6.4-pixel",
+  validateTargetProfile("-Tps_6_4", "dxilv1.4--shadermodel6.4-pixel",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tcs_6_5", "dxil--shadermodel6.5-compute",
+  validateTargetProfile("-Tcs_6_5", "dxilv1.5--shadermodel6.5-compute",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tms_6_6", "dxil--shadermodel6.6-mesh",
+  validateTargetProfile("-Tms_6_6", "dxilv1.6--shadermodel6.6-mesh",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tas_6_7", "dxil--shadermodel6.7-amplification",
+  validateTargetProfile("-Tas_6_7", "dxilv1.7--shadermodel6.7-amplification",
                         InMemoryFileSystem, Diags);
-  validateTargetProfile("-Tlib_6_x", "dxil--shadermodel6.15-library",
+  validateTargetProfile("-Tcs_6_8", "dxilv1.8--shadermodel6.8-compute",
+                        InMemoryFileSystem, Diags);
+  validateTargetProfile("-Tlib_6_x", "dxilv1.8--shadermodel6.15-library",
                         InMemoryFileSystem, Diags);
 
   // Invalid tests.
diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 7da30e6cf96f..4e357cddcf2a 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -176,6 +176,7 @@ class Triple {
     DXILSubArch_v1_6,
     DXILSubArch_v1_7,
     DXILSubArch_v1_8,
+    LatestDXILSubArch = DXILSubArch_v1_8,
   };
   enum VendorType {
     UnknownVendor,
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index aa8160d18edd..50f8d6ec8420 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -152,8 +152,8 @@ struct VerifierSupport {
   bool TreatBrokenDebugInfoAsError = true;
 
   explicit VerifierSupport(raw_ostream *OS, const Module &M)
-      : OS(OS), M(M), MST(&M), TT(M.getTargetTriple()), DL(M.getDataLayout()),
-        Context(M.getContext()) {}
+      : OS(OS), M(M), MST(&M), TT(Triple::normalize(M.getTargetTriple())),
+        DL(M.getDataLayout()), Context(M.getContext()) {}
 
 private:
   void Write(const Module *M) {
diff --git a/llvm/lib/TargetParser/Triple.cpp b/llvm/lib/TargetParser/Triple.cpp
index f3f244c814e7..35686c1ea900 100644
--- a/llvm/lib/TargetParser/Triple.cpp
+++ b/llvm/lib/TargetParser/Triple.cpp
@@ -115,6 +115,31 @@ StringRef Triple::getArchName(ArchType Kind, SubArchType SubArch) {
     if (SubArch == AArch64SubArch_arm64e)
       return "arm64e";
     break;
+  case Triple::dxil:
+    switch (SubArch) {
+    case Triple::NoSubArch:
+    case Triple::DXILSubArch_v1_0:
+      return "dxilv1.0";
+    case Triple::DXILSubArch_v1_1:
+      return "dxilv1.1";
+    case Triple::DXILSubArch_v1_2:
+      return "dxilv1.2";
+    case Triple::DXILSubArch_v1_3:
+      return "dxilv1.3";
+    case Triple::DXILSubArch_v1_4:
+      return "dxilv1.4";
+    case Triple::DXILSubArch_v1_5:
+      return "dxilv1.5";
+    case Triple::DXILSubArch_v1_6:
+      return "dxilv1.6";
+    case Triple::DXILSubArch_v1_7:
+      return "dxilv1.7";
+    case Triple::DXILSubArch_v1_8:
+      return "dxilv1.8";
+    default:
+      break;
+    }
+    break;
   default:
     break;
   }
@@ -1014,6 +1039,53 @@ Triple::Triple(const Twine &ArchStr, const Twine &VendorStr, const Twine &OSStr,
     ObjectFormat = getDefaultFormat(*this);
 }
 
+static VersionTuple parseVersionFromName(StringRef Name);
+
+static StringRef getDXILArchNameFromShaderModel(StringRef ShaderModelStr) {
+  VersionTuple Ver =
+      parseVersionFromName(ShaderModelStr.drop_front(strlen("shadermodel")));
+  // Default DXIL minor version when Shader Model version is anything other
+  // than 6.[0...8] or 6.x (which translates to latest current SM version)
+  const unsigned SMMajor = 6;
+  if (!Ver.empty()) {
+    if (Ver.getMajor() == SMMajor) {
+      if (std::optional<unsigned> SMMinor = Ver.getMinor()) {
+        switch (*SMMinor) {
+        case 0:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_0);
+        case 1:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_1);
+        case 2:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_2);
+        case 3:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_3);
+        case 4:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_4);
+        case 5:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_5);
+        case 6:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_6);
+        case 7:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_7);
+        case 8:
+          return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_8);
+        default:
+          report_fatal_error("Unsupported Shader Model version", false);
+        }
+      }
+    }
+  } else {
+    // Special case: DXIL minor version is set to LatestCurrentDXILMinor for
+    // shadermodel6.x is
+    if (ShaderModelStr == "shadermodel6.x") {
+      return Triple::getArchName(Triple::dxil, Triple::LatestDXILSubArch);
+    }
+  }
+  // DXIL version corresponding to Shader Model version other than 6.Minor
+  // is 1.0
+  return Triple::getArchName(Triple::dxil, Triple::DXILSubArch_v1_0);
+}
+
 std::string Triple::normalize(StringRef Str) {
   bool IsMinGW32 = false;
   bool IsCygwin = false;
@@ -1206,6 +1278,20 @@ std::string Triple::normalize(StringRef Str) {
     }
   }
 
+  // Normalize DXIL triple if it does not include DXIL version number.
+  // Determine DXIL version number using the minor version number of Shader
+  // Model version specified in target triple, if any. Prior to decoupling DXIL
+  // version numbering from that of Shader Model DXIL version 1.Y corresponds to
+  // SM 6.Y. E.g., dxilv1.Y-unknown-shadermodelX.Y-hull
+  if (Components[0] == "dxil") {
+    if (Components.size() > 4) {
+      Components.resize(4);
+    }
+    // Add DXIL version only if shadermodel is specified in the triple
+    if (OS == Triple::ShaderModel) {
+      Components[0] = getDXILArchNameFromShaderModel(Components[2]);
+    }
+  }
   // Stick the corrected components back together to form the normalized string.
   return join(Components, "-");
 }
diff --git a/llvm/unittests/TargetParser/TripleTest.cpp b/llvm/unittests/TargetParser/TripleTest.cpp
index b8f5fbd87407..3112014d9efb 100644
--- a/llvm/unittests/TargetParser/TripleTest.cpp
+++ b/llvm/unittests/TargetParser/TripleTest.cpp
@@ -2454,4 +2454,20 @@ TEST(TripleTest, isArmMClass) {
     EXPECT_TRUE(T.isArmMClass());
   }
 }
+
+TEST(TripleTest, DXILNormaizeWithVersion) {
+  EXPECT_EQ("dxilv1.0-unknown-shadermodel6.0",
+            Triple::normalize("dxilv1.0--shadermodel6.0"));
+  EXPECT_EQ("dxilv1.0-unknown-shadermodel6.0",
+            Triple::normalize("dxil--shadermodel6.0"));
+  EXPECT_EQ("dxilv1.1-unknown-shadermodel6.1-library",
+            Triple::normalize("dxil-shadermodel6.1-unknown-library"));
+  EXPECT_EQ("dxilv1.8-unknown-shadermodel6.x-unknown",
+            Triple::normalize("dxil-unknown-shadermodel6.x-unknown"));
+  EXPECT_EQ("dxilv1.8-unknown-shadermodel6.x-unknown",
+            Triple::normalize("dxil-unknown-shadermodel6.x-unknown"));
+  EXPECT_EQ("dxil-unknown-unknown-unknown", Triple::normalize("dxil---"));
+  EXPECT_EQ("dxilv1.0-pc-shadermodel5.0-compute",
+            Triple::normalize("dxil-shadermodel5.0-pc-compute"));
+}
 } // end anonymous namespace

Copy link
Contributor

@dmpots dmpots left a comment

Choose a reason for hiding this comment

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

It would be nice to mention in the description what the sanitizer fix was compared to the original PR.

@bharadwajy
Copy link
Contributor Author

It would be nice to mention in the description what the sanitizer fix was compared to the original PR.

Thanks! The fix to address ASAN failure is in commit 8522e36 of this PR. Updated the description as well.

@bogner
Copy link
Contributor

bogner commented May 8, 2024

It would be nice to mention in the description what the sanitizer fix was compared to the original PR.

Thanks! The fix to address ASAN failure is in commit 8522e36 of this PR. Updated the description as well.

Since we squash commits when we merge it doesn't really make sense to refer to the hash of the PR in the description - this won't point at anything meaningful once the change is merged and the PR commits get garbage collected.

@bharadwajy bharadwajy merged commit 6d89014 into llvm:main May 8, 2024
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:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category HLSL HLSL Language Support llvm:ir
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants