[PATCH] D133983: [HLSL] Add SV_DispatchThreadID

Thu Sep 22 15:14:05 PDT 2022

python3kgae added a comment.

In D133983#3808302 <https://reviews.llvm.org/D133983#3808302>, @aaron.ballman wrote:

> In D133983#3807284 <https://reviews.llvm.org/D133983#3807284>, @python3kgae wrote:
>
>> In D133983#3805761 <https://reviews.llvm.org/D133983#3805761>, @aaron.ballman wrote:
>>
>>> There are no tests for applying this to a global variable, so those should be added.
>>
>> The global variable in the Subjects is wrong.
>> It should be Field.
>
> I wondered if that was the case, thanks for the fix. :-)
>
>> Support for semantic on field is a bigger change.
>> Created https://github.com/llvm/llvm-project/issues/57889 to track it.
>
> Thanks!
>
> You should add additional test coverage: applying the attribute to a static data member, to a non-static data member, to a variable, to an unnamed parameter, and to a lambda parameter. The tests should demonstrate that we give good diagnostics where appropriate, instead of crashing, asserting, or silently accepting.

Added test which only gets error now.
Create issue https://github.com/llvm/llvm-project/issues/57916 to change the error to a warning.
lambda is not supported in HLSL, so not add test for the lambda parameter.

================
Comment at: clang/include/clang/Basic/AttrDocs.td:6596-6598
+The ``SV_DispatchThreadID`` semantic, when applied to an input parameter, specifies a
+data binding to map global thread offset within the Dispatch call(per dimension of the group) to the specified parameter.
+When applied to a field of a struct, the data binding is specified to the field when the struct is used as a parameter type.
----------------
aaron.ballman wrote:
> You should also wrap the docs to the usual 80-col limit.
> 
> Can you apply the attribute to a static data member in a struct, or only fields?
Not sure if static data member in a struct should be allowed for HLSL or not yet.

If it is allowed, it should be ignored with a warning. The issue is tracked by issue https://github.com/llvm/llvm-project/issues/57916

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133983