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

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:43
+    return false;
+  if (!Version.getMinor())
+    return false;
----------------
python3kgae wrote:
> 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.
That's for the option handling code path where we enforce that it is always set by the parsing code. There's no reason we need to require the minor version if the triple is explicitly provided.


================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:57
+  case Triple::EnvironmentType::Compute: {
+    if (isLegalVersion(Version, 4, 0, 1))
+      return true;
----------------
python3kgae wrote:
> 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.
I don't think we need to bump the value in this code.

My point is that targeting ShaderModel 4.2 isn't going to actually break anything in the compiler, and the rigidity of this code means it will need to be updated every time we update and alter shader models.

It should be sufficient to verify that a Compute shader is targeting shader model >= 4.0, we shouldn't need to verify that it is 4.0, 4.1, 5.0, 5.1 or 6.0-6.7. That's a lot of code to maintain for no real requirement.


================
Comment at: clang/lib/Driver/ToolChains/HLSL.h:28
+
+  std::string ComputeEffectiveClangTriple(const llvm::opt::ArgList &Args,
+                                          types::ID InputType) const override;
----------------
python3kgae wrote:
> 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.
Doh! Please ignore :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122865