[PATCH] D158820: [Sema][HLSL] Fix naming of anyhit/closesthit shaders

[Justin Bogner via Phabricator via cfe-commits]

bogner added inline comments.


================
Comment at: clang/test/SemaHLSL/entry_shader.hlsl:1
-// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-compute -x hlsl -hlsl-entry foo  -o - %s -DSHADER='"anyHit"' -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-compute -x hlsl -hlsl-entry foo  -o - %s -DSHADER='"mesh"' -verify
 // RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-compute -x hlsl -hlsl-entry foo  -o - %s -DSHADER='"compute"'
----------------
bob80905 wrote:
> Why was this changed to mesh instead of "anyhit" ?
Another case of trying to keep things to one error - `anyhit` doesn't support the `numthreads` attribute


================
Comment at: clang/test/SemaHLSL/shader_type_attr.hlsl:30
 
 // expected-error at +1 {{'shader' attribute parameters do not match the previous declaration}}
+[shader("pixel")]
----------------
beanz wrote:
> bob80905 wrote:
> > bogner wrote:
> > > bob80905 wrote:
> > > > I don't think we should expect this error, given that there is no previous declaration for doubledUp(). Maybe something else like %0 and %1 are incompatible attributes? 
> > > I don't necessarily disagree, but if we're going to change that I think it should be in a different change than this one.
> > I'm also curious, why is the change from compute to pixel necessary here?
> I suspect this change is just to prevent the presence of the `compute` annotation from triggering the `missing numthreads` error.
That's correct - just trying to keep to testing the conflicting attribute error and not have a spurious numthreads as well.


================
Comment at: clang/test/SemaHLSL/shader_type_attr.hlsl:61
 // CHECK:HLSLShaderAttr 0x{{[0-9a-fA-F]+}} <line:61:2, col:18> Compute
-[shader("compute")]
+[shader("compute"), numthreads(8,1,1)]
 int entry() {
----------------
beanz wrote:
> Oh interesting! DXC doesn't actually support this syntax, but I think it is worth supporting in Clang.
> Should we also check for the numthreads attribute?
Hm, it might be better to stick to the syntax dxc handles for these tests at least. I'll update that. Sure, checking both attributes seems like a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158820