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 | ||
|---|---|---|
| 6596–6598 | 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 | ||
| 6928 | 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 | ||
|---|---|---|
| 6596–6598 | The global variable is wrong. Change it to Field and added document. | |
| clang/lib/Sema/SemaDeclAttr.cpp | ||
| 6928 | 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 | ||
|---|---|---|
| 6596–6598 | 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 | ||
| clang/lib/Sema/SemaDeclAttr.cpp | ||
| 6916 | ||
| 6940 | 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. | |
| 6943 | ||
| clang/test/SemaHLSL/Semantics/entry_parameter.hlsl | ||
| 3–4 | ||
| clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl | ||
| 4–14 | ||
| clang/test/SemaHLSL/Semantics/valid_entry_parameter.hlsl | ||
| 4–17 | ||
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 | ||
|---|---|---|
| 6596–6598 | 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 | ||
|---|---|---|
| 6597 | Sorry, I had a typo in my earlier suggestion! | |
| clang/lib/CodeGen/CGHLSLRuntime.cpp | ||
|---|---|---|
| 167 | I might be missing something, but I'm not seeing a test that exercises the sret case. | |
| clang/lib/CodeGen/CGHLSLRuntime.cpp | ||
|---|---|---|
| 167 | Issue https://github.com/llvm/llvm-project/issues/57874 is to track this. | |
| clang/lib/CodeGen/CGHLSLRuntime.cpp | ||
|---|---|---|
| 167 | My point is that if this code can't be reached and tested, we shouldn't add it. | |
| clang/lib/CodeGen/CGHLSLRuntime.cpp | ||
|---|---|---|
| 167 | Added a test. | |
Sorry, I had a typo in my earlier suggestion!