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

[Justin Bogner via Phabricator via cfe-commits]

bogner added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:710-716
+  "Shader model is required in target '%0' for DXIL generation">;
+def err_drv_dxil_unsupported_shader_model : Error<
+  "Shader model '%0' in target '%1' is invalid for DXIL generation">;
+def err_drv_dxil_missing_shader_stage : Error<
+  "Shader stage is required in target '%0' for DXIL generation">;
+def err_drv_dxil_unsupported_shader_stage : Error<
+  "Shader stage '%0' in target '%1' is invalid for DXIL generation">;
----------------
aaron.ballman wrote:
> 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">;
> ```
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?



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4054
+      if (T.getOSName().empty()) {
+        Diags.Report(diag::err_drv_dxil_missing_shader_model) << T.str();
+      } else if (!T.isShaderModelOS() || T.getOSVersion() == VersionTuple(0)) {
----------------
aaron.ballman wrote:
> 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).
Tried it, and no. Looks like we don't have that overload.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159103