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

[Xiang Li via Phabricator via cfe-commits]

python3kgae added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:43
+    return false;
+  if (!Version.getMinor())
+    return false;
----------------
beanz wrote:
> Do we need to verify this or can we just assume if there is no minor version specified that the version is 0?
To match things like ps_6_0, it would be nice to require minor version.


================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:57
+  case Triple::EnvironmentType::Compute: {
+    if (isLegalVersion(Version, 4, 0, 1))
+      return true;
----------------
beanz wrote:
> 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.
I could add the code to bump to 6.0, then we don't need to validator lower shader model like 4_1 here.


================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:62
+
+    if (isLegalVersion(Version, 6, 0, MaxShaderModel6Minor))
+      return true;
----------------
beanz wrote:
> 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.
The chance to hit this error should be very small.
Should be OK to report it in validation only.


================
Comment at: clang/lib/Driver/ToolChains/HLSL.h:28
+
+  std::string ComputeEffectiveClangTriple(const llvm::opt::ArgList &Args,
+                                          types::ID InputType) const override;
----------------
beanz wrote:
> nit: function names should start with a lowercase letter:
> 
> https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
This is an override.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122865