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