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