Support SV_DispatchThreadID attribute.
Translate it into dx.thread.id in clang codeGen.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are no tests for applying this to a global variable, so those should be added.
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
6573–6575 | It doesn't specify what happens when applied to a global variable. | |
clang/lib/CodeGen/CGHLSLRuntime.cpp | ||
113 | ||
120–121 | ||
clang/lib/Sema/SemaDeclAttr.cpp | ||
6925 | Are there other restrictions we need to care about, like what the type of the parameter/global is, etc? |
The global variable in the Subjects is wrong.
It should be Field.
Support for semantic on field is a bigger change.
Created https://github.com/llvm/llvm-project/issues/57889 to track it.
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
6573–6575 | The global variable is wrong. Change it to Field and added document. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
6925 | Yes. Added the check for type. |
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.
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
6573–6575 | 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? | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
11656 ↗ | (On Diff #462039) | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
6913 | ||
6937 | This will cause an assert when the attribute appears on a field -- you need to check for a field decl above and give a diagnostic about it not being supported yet. | |
6940 | ||
clang/test/SemaHLSL/Semantics/entry_parameter.hlsl | ||
4 ↗ | (On Diff #462039) | |
clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl | ||
3–13 ↗ | (On Diff #462039) | |
clang/test/SemaHLSL/Semantics/valid_entry_parameter.hlsl | ||
3–16 ↗ | (On Diff #462039) |
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.
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
6573–6575 | 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 |
LGTM aside from a think-o that I introduced with an earlier suggested change. I'm not super qualified for the CodeGen changes. They look correct to me, but @beanz would probably be able to better validate the correctness there.
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
6574 | Sorry, I had a typo in my earlier suggestion! |
clang/lib/CodeGen/CGHLSLRuntime.cpp | ||
---|---|---|
166 | I might be missing something, but I'm not seeing a test that exercises the sret case. |
clang/lib/CodeGen/CGHLSLRuntime.cpp | ||
---|---|---|
166 | Issue https://github.com/llvm/llvm-project/issues/57874 is to track this. |
clang/lib/CodeGen/CGHLSLRuntime.cpp | ||
---|---|---|
166 | My point is that if this code can't be reached and tested, we shouldn't add it. |
clang/lib/CodeGen/CGHLSLRuntime.cpp | ||
---|---|---|
166 | Added a test. |
Sorry, I had a typo in my earlier suggestion!