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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
