[PATCH] D122865: [HLSL][clang][Driver] Support target profile command line option.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added reviewers: pow2clk, rnk, bogner, MaskRay, dexonsmith.
beanz added a comment.
Herald added a subscriber: StephenFan.

Looping in a few extra reviewers, also added some comments below.



================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:43
+    return false;
+  if (!Version.getMinor())
+    return false;
----------------
Do we need to verify this or can we just assume if there is no minor version specified that the version is 0?


================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:57
+  case Triple::EnvironmentType::Compute: {
+    if (isLegalVersion(Version, 4, 0, 1))
+      return true;
----------------
Do we actually care if someone sets a compute shader as targeting shader model 4.8 (a version that doesn't really exist)?

For compatibility with DXC, when targeting DXIL we're going to end up bumping older shader model targets up to 6.0 anyways, so I'm not sure it makes sense to verify at this level.

I think it is sufficient to just verify that the version is >= the shader model that introduced the stage.


================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:62
+
+    if (isLegalVersion(Version, 6, 0, MaxShaderModel6Minor))
+      return true;
----------------
One down side to this type of version matching is that when new shader model versions are introduced all of this code needs to be updated.

If we don't need this in-depth verification we should leave it out. I suspect drivers providing runtime verification, and the dxil verification we do later means we can leave this off.


================
Comment at: clang/lib/Driver/ToolChains/HLSL.h:28
+
+  std::string ComputeEffectiveClangTriple(const llvm::opt::ArgList &Args,
+                                          types::ID InputType) const override;
----------------
nit: function names should start with a lowercase letter:

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122865