[Driver][HLSL] Improve diagnostics for invalid shader model and stage

[bogner]

Summary

This adds more validation that a dxil triple is actually useable when
compiling HLSL.

The OS field of the triple needs to be a versioned shader model.
Later, we should set a default if this is empty and check that the
version is a shader model we can actually handle.

The Environment field of the triple needs to be specified and be a
valid shader stage. I'd like to allow this to be empty and treat it
like library, but allowing that currently crashes in DXIL metadata
handling.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticDriverKinds.td

710–716

Hmmm in theory:

def err_drv_dxil_bad_shader_required_in_target : Error<
  "shader %select{model|stage}0 is required in target '%1' for DXIL generation">;

def err_drv_dxil_bad_shader_unsupported : Error<
  "shader %select{model|stage}0 '%1' in target '%2' is invalid for DXIL generation">;

clang/lib/Frontend/CompilerInvocation.cpp

4148

Idle question: can you pass T directly here? (I thought we had diagnostic streaming support for Triple but I'm not spotting it with a quick look through the source).

[bogner]

clang/include/clang/Basic/DiagnosticDriverKinds.td

710–716

To make that readable you kind of have to do something like this:

if (T.isDXIL()) {
  enum { ShaderModel, ShaderStage };
  if (T.getOSName().empty()) {
    Diags.Report(diag::err_drv_dxil_bad_shader_required_in_target)
        << ShaderModel << T.str();
  } else if (!T.isShaderModelOS() || T.getOSVersion() == VersionTuple(0)) {
    Diags.Report(diag::err_drv_dxil_bad_shader_invalid)
        ShaderModel << T.getOSName() << T.str();
  } else if (T.getEnvironmentName().empty()) {
    Diags.Report(diag::err_drv_dxil_bad_shader_required_in_target)
        << ShaderStage << T.str();
  } else if (!T.isShaderStageEnvironment()) {
    Diags.Report(diag::err_drv_dxil_bad_shader_invalid)
        << ShaderStage << T.getEnvironmentName() << T.str();
  }
}

It's not really clear to me which way is better. WDYT?

clang/lib/Frontend/CompilerInvocation.cpp

4148

Tried it, and no. Looks like we don't have that overload.

[bogner]

• Consolidate error messages
• Error messages should start with a lower case letter

[bogner]

Ping. Please take another look when you get a chance, thanks!

[bogner]

Rebased to fix conflicts

[bogner]

clang/lib/Frontend/CompilerInvocation.cpp

4146

@Keenuts Does this look right, or do we want something like T.isSPIRVLogical here?

[Keenuts]

clang/lib/Frontend/CompilerInvocation.cpp

4146

Thanks for asking!
I think isSPIRVLogical() would be better here, as other SPIR-V flavors are not compatible with graphics (including compute shaders).
Ideally you could also add a TODO: revisit this once we figured out how to handle PhysicalStorageBuffer64 memory model.

[bogner]

• Check isSPIRVLogical rather than isSPIRV
• Improve diagnostics given that HLSL can generate both DXIL and SPIR-V
• Add tests for SPIR-V triples
• Fix missing REQUIRES: in dxil triple tests

[Keenuts]

LGTM from the SPIR-V side. Thanks!

[aaron.ballman]

LGTM!

clang/include/clang/Basic/DiagnosticDriverKinds.td

def err_drv_target_variant_invalid : Error<
   "unsupported '%0' value '%1'; use 'ios-macabi' instead">;
 
 def err_drv_invalid_directx_shader_module : Error<
   "invalid profile : %0">;
 def err_drv_dxc_missing_target_profile : Error<
   "target profile option (-T) is missing">;
 def err_drv_hlsl_unsupported_target : Error<
   "HLSL code generation is unsupported for target '%0'">;
+def err_drv_dxil_bad_shader_required_in_target : Error<
+  "shader %select{model|stage}0 is required in target '%1' for DXIL generation">;
+
+def err_drv_dxil_bad_shader_unsupported : Error<
+  "shader %select{model|stage}0 '%1' in target '%2' is invalid for DXIL generation">;
 def warn_drv_dxc_missing_dxv : Warning<"dxv not found. "
     "Resulting DXIL will not be validated or signed for use in release environments.">,
     InGroup<DXILValidation>;
 
 def err_drv_invalid_range_dxil_validator_version : Error<
   "invalid validator version : %0\n"
   "Validator version must be less than or equal to current internal version.">;
 def err_drv_invalid_format_dxil_validator_version : Error<
   "invalid validator version : %0\n"
   "Format of validator version is \"<major>.<minor>\" (ex:\"1.4\").">;
 def err_drv_invalid_empty_dxil_validator_version : Error<

clang/lib/Frontend/CompilerInvocation.cpp

if (const Arg *A = Args.getLastArg(OPT_frandomize_layout_seed_file_EQ)) {
     std::getline(SeedFile, Opts.RandstructSeed);
   }
 
   if (const Arg *A = Args.getLastArg(OPT_frandomize_layout_seed_EQ))
     Opts.RandstructSeed = A->getValue(0);
 
   // Validate options for HLSL
   if (Opts.HLSL) {
-    bool SupportedTarget = (T.getArch() == llvm::Triple::dxil ||
-                            T.getArch() == llvm::Triple::spirv) &&
-                           T.getOS() == llvm::Triple::ShaderModel;
-    if (!SupportedTarget)
+    if (T.isDXIL() || T.isSPIRV) {
+      enum { ShaderModel, ShaderStage };
+      if (T.getOSName().empty()) {
+        Diags.Report(diag::err_drv_dxil_bad_shader_required_in_target)
+            << ShaderModel << T.str();
+      } else if (!T.isShaderModelOS() || T.getOSVersion() == VersionTuple(0)) {
+        Diags.Report(diag::err_drv_dxil_bad_shader_unsupported)
+            << ShaderModel << T.getOSName() << T.str();
+      } else if (T.getEnvironmentName().empty()) {
+        Diags.Report(diag::err_drv_dxil_bad_shader_required_in_target)
+            << ShaderStage << T.str();
+      } else if (!T.isShaderStageEnvironment()) {
+        Diags.Report(diag::err_drv_dxil_bad_shader_unsupported)
+            << ShaderStage << T.getEnvironmentName() << T.str();
+      }
+    } else
       Diags.Report(diag::err_drv_hlsl_unsupported_target) << T.str();
   }
 
   return Diags.getNumErrors() == NumErrorsBefore;
 }
 
 static bool isStrictlyPreprocessorAction(frontend::ActionKind Action) {
   switch (Action) {

clang/lib/Sema/SemaDecl.cpp

                                                  FD->getBeginLoc()));
     }
   } else {
     switch (TargetInfo.getTriple().getEnvironment()) {
     case llvm::Triple::UnknownEnvironment:
     case llvm::Triple::Library:
       break;
     default:
-      // TODO: This should probably just be llvm_unreachable and we should
-      // reject triples with random ABIs and such when we build the target.
-      // For now, crash.
-      llvm::report_fatal_error("Unhandled environment in triple");
+      llvm_unreachable("Unhandled environment in triple");
     }
   }
 }
 
 void Sema::CheckHLSLEntryPoint(FunctionDecl *FD) {
   const auto *ShaderAttr = FD->getAttr<HLSLShaderAttr>();
   assert(ShaderAttr && "Entry point has no shader attribute");
   HLSLShaderAttr::ShaderType ST = ShaderAttr->getType();

clang/test/Driver/hlsl-lang-targets.hlsl

-// RUN: not %clang -target x86_64-unknown-unknown %s 2>&1 | FileCheck %s --check-prefix=X86
-// RUN: not %clang -target dxil-unknown-unknown %s 2>&1 | FileCheck %s --check-prefix=DXIL
-// RUN: not %clang -target x86_64-unknown-shadermodel %s 2>&1 | FileCheck %s --check-prefix=SM
-// RUN: not %clang -target spirv64-unknown-unknown %s 2>&1 | FileCheck %s --check-prefix=SPIRV
+// Supported targets
+//
+// RUN: %clang -target dxil--shadermodel6.2-pixel %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-VALID %s
+// RUN: %clang -target dxil-unknown-shadermodel6.2-pixel %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-VALID %s
+// RUN: %clang -target dxil--shadermodel6.2-library %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-VALID %s
+// RUN: %clang -target dxil-unknown-shadermodel6.2-library %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-VALID %s
 
+// Empty shader model
+//
+// RUN: not %clang -target dxil %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-NO-OS %s
 
-// A completely unsupported target...
-// X86: error: HLSL code generation is unsupported for target 'x86_64-unknown-unknown'
+// Invalid shader models
+//
+// RUN: not %clang -target dxil--linux %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-OS %s
+// RUN: not %clang -target dxil--win32 %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-OS %s
+// RUN: not %clang -target dxil--unknown %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-OS %s
+// RUN: not %clang -target dxil--invalidos %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-OS %s
 
-// Poorly specified targets
-// DXIL: error: HLSL code generation is unsupported for target 'dxil-unknown-unknown'
-// SM: error: HLSL code generation is unsupported for target 'x86_64-unknown-shadermodel'
+// Bad shader model versions. Currently we just check for any version at all.
+//
+// RUN: not %clang -target dxil--shadermodel %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-OS %s
+// RUN: not %clang -target dxil--shadermodel0.0 %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-OS %s
 
-// FIXME// SPIRV: error: HLSL code generation is unsupported for target 'spirv64-unknown-unknown'
+// Empty shader stage
+//
+// RUN: not %clang -target dxil-shadermodel6.2 %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-NO-ENV %s
+// RUN: not %clang -target dxil--shadermodel6.2 %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-NO-ENV %s
+// RUN: not %clang -target dxil--shadermodel6.2 %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-NO-ENV %s
+
+// Invalid shader stages
+//
+// RUN: not %clang -target dxil--shadermodel6.2-unknown %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-ENV %s
+// RUN: not %clang -target dxil--shadermodel6.2-invalidenvironment %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-ENV %s
+// RUN: not %clang -target dxil--shadermodel6.2-eabi %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-ENV %s
+// RUN: not %clang -target dxil--shadermodel6.2-msvc %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-ENV %s
+
+// Non-dxil targets
+//
+// RUN: not %clang -target x86_64-unknown-unknown %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-TARGET %s
+// RUN: not %clang -target x86_64-linux %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-TARGET %s
+// RUN: not %clang -target amdgcn %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-TARGET %s
+// RUN: not %clang -target spirv64-unknown-unknown %s -S -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BAD-TARGET %s
+
+// CHECK-VALID-NOT: error:
+// CHECK-NO-OS: error: shader model is required in target '{{.*}}' for DXIL generation
+// CHECK-BAD-OS: error: shader model '{{.*}}' in target '{{.*}}' is invalid for DXIL generation
+// CHECK-NO-ENV: error: shader stage is required in target '{{.*}}' for DXIL generation
+// CHECK-BAD-ENV: error: shader stage '{{.*}}' in target '{{.*}}' is invalid for DXIL generation
+// CHECK-BAD-TARGET: error: HLSL code generation is unsupported for target '{{.*}}'
+
+[shader("pixel")]
+void main() {}

llvm/include/llvm/TargetParser/Triple.h

enum EnvironmentType {
     RayGeneration,
     Intersection,
     AnyHit,
     ClosestHit,
     Miss,
     Callable,
     Mesh,
     Amplification,
+
     OpenHOS,
+
     LastEnvironmentType = OpenHOS
   };
   enum ObjectFormatType {
     UnknownObjectFormat,
 
     COFF,
     DXContainer,
     ELF,
public:
 
   bool isOSLiteOS() const { return getOS() == Triple::LiteOS; }
 
   /// Tests whether the target is DXIL.
   bool isDXIL() const {
     return getArch() == Triple::dxil;
   }
 
+  bool isShaderModelOS() const {
+    return getOS() == Triple::ShaderModel;
+  }
+
+  bool isShaderStageEnvironment() const {
+    EnvironmentType Env = getEnvironment();
+    return Env == Triple::Pixel || Env == Triple::Vertex ||
+           Env == Triple::Geometry || Env == Triple::Hull ||
+           Env == Triple::Domain || Env == Triple::Compute ||
+           Env == Triple::Library || Env == Triple::RayGeneration ||
+           Env == Triple::Intersection || Env == Triple::AnyHit ||
+           Env == Triple::ClosestHit || Env == Triple::Miss ||
+           Env == Triple::Callable || Env == Triple::Mesh ||
+           Env == Triple::Amplification;
+  }
+
   /// Tests whether the target is SPIR (32- or 64-bit).
   bool isSPIR() const {
     return getArch() == Triple::spir || getArch() == Triple::spir64;
   }
 
   /// Tests whether the target is SPIR-V (32/64-bit/Logical).
   bool isSPIRV() const {
     return getArch() == Triple::spirv32 || getArch() == Triple::spirv64 ||