This is an archive of the discontinued LLVM Phabricator instance.

[HLSL] Add SV_DispatchThreadID
ClosedPublic

Authored by python3kgae on Sep 15 2022, 3:51 PM.

Details

Summary

Support SV_DispatchThreadID attribute.
Translate it into dx.thread.id in clang codeGen.

Diff Detail

Event Timeline

python3kgae created this revision.Sep 15 2022, 3:51 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: Anastasia. · View Herald Transcript
python3kgae requested review of this revision.Sep 15 2022, 3:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 3:51 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Fix warning.

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?

python3kgae marked 4 inline comments as done.

Add type check for dispatch thread id.

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.
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.
But adding support for field is a big change.
Created https://github.com/llvm/llvm-project/issues/57889 to track it.

clang/lib/Sema/SemaDeclAttr.cpp
6928

Yes. Added the check for type.

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.

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
python3kgae marked 8 inline comments as done.

Add more tests.

Add newline at end of file

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.

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

aaron.ballman accepted this revision.Sep 29 2022, 12:19 PM

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!

This revision is now accepted and ready to land.Sep 29 2022, 12:19 PM
python3kgae marked an inline comment as done.

Fix typo.

beanz added inline comments.Sep 30 2022, 12:47 PM
clang/lib/CodeGen/CGHLSLRuntime.cpp
167

I might be missing something, but I'm not seeing a test that exercises the sret case.

python3kgae marked an inline comment as done.Sep 30 2022, 12:52 PM
python3kgae added inline comments.
clang/lib/CodeGen/CGHLSLRuntime.cpp
167

Issue https://github.com/llvm/llvm-project/issues/57874 is to track this.
Cannot create a legal test on compute shader which only has input.

beanz added inline comments.Sep 30 2022, 12:53 PM
clang/lib/CodeGen/CGHLSLRuntime.cpp
167

My point is that if this code can't be reached and tested, we shouldn't add it.

python3kgae marked an inline comment as done.

Insert position value for sret output to make a test.

python3kgae added inline comments.Sep 30 2022, 2:03 PM
clang/lib/CodeGen/CGHLSLRuntime.cpp
167

Added a test.
It is still illegal HLSL, marked FIXME in the test.

beanz added inline comments.Oct 7 2022, 10:52 AM
clang/lib/CodeGen/CGHLSLRuntime.cpp
135

nit: this comment doesn't add value

166

Was dropping the const here intentional? Getting the parameter's type shouldn't modify it.

clang/test/CodeGenHLSL/sret_output.hlsl
5
18

add newline to the end of the file

python3kgae marked 5 inline comments as done.

Fix test fail after rebase and code cleanup to match comments.

Check sret parameter exist.

More check on the ir.

beanz accepted this revision.Oct 18 2022, 2:43 PM

LGTM

This revision was automatically updated to reflect the committed changes.