This is an archive of the discontinued LLVM Phabricator instance.

[HLSL] Add resource binding attribute for HLSL.
ClosedPublic

Authored by python3kgae on Jul 18 2022, 11:59 AM.

Details

Summary

The resource binding attribute is to set the virtual registers and logical register spaces resources in HLSL are bound to.
Format is ''register(ID, space)'' like register(t3, space1).
ID must be start with
t – for shader resource views (SRV),
s – for samplers,
u – for unordered access views (UAV),
b – for constant buffer views (CBV).

Register space is default to space0.

The full documentation is available here: https://docs.microsoft.com/en-us/windows/win32/direct3d12/resource-binding-in-hlsl

Diff Detail

Event Timeline

python3kgae created this revision.Jul 18 2022, 11:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 11:59 AM
python3kgae requested review of this revision.Jul 18 2022, 11:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 11:59 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
beanz added a comment.Aug 8 2022, 11:22 AM

I have some nitpick comments about the wording of the errors and docs, but the bigger meta issue is that you've implemented this parsing entirely from scratch, and I suspect this should be sharing parsing logic with the generic semantic parser.

I know the parameters themselves have special parsing behavior, but it seems to me that we could parse this as a normal attribute and verify the arguments post attribute parsing.

clang/include/clang/Basic/Attr.td
4054

ID seems like the wrong name here as it doesn't really convey the meaning of the value. Maybe register or slot?

clang/include/clang/Basic/AttrDocs.td
6596
6597
6598
6604
clang/include/clang/Basic/DiagnosticParseKinds.td
1622
1623
1624
clang/include/clang/Parse/Parser.h
2821 ↗(On Diff #445588)

I don't think this should be fully separate from parsing other HLSL semantics.

clang/lib/Parse/ParseHLSL.cpp
205

register isn't a kw.

Move resource binding attribute validation to Sema.

Treat resource binding as "HLSLSemantic".

aaron.ballman added inline comments.Aug 10 2022, 7:05 AM
clang/include/clang/Basic/Attr.td
150

This string literal looks suspiciously wrong to me. This is the text that shows up in a diagnostic message, so it's important to get it correct; so it seems like there's missing test coverage for the attribute.

4051

Do you really intend Slot to be optional?

clang/include/clang/Basic/AttrDocs.td
6597

This documentation says slot is mandatory, hence the question above about the argument.

6608

I think it would help to have a working example here that shows both slot without space and slot with space.

clang/include/clang/Basic/DiagnosticParseKinds.td
1622

Unintended whitespace change.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11669

This diagnostic is not necessary; the common attribute handler already covers this. You're missing test coverage to verify that though.

11670

Will a user be able to determine how to fix the issue from this? Mostly, will it be clear what the "resource class specifier" is that they have incorrect? Perhaps it would help to say invalid resource class specifier '%0' used; expected 'b', 's', 't', or 'u'?

11672–11673

I don't know what the difference is between these two diagnostics; they seem to be the same situation. Also 'spaceX' looks like syntax the user wrote, but it's hardcoded in the diagnostic. Can these be replaced with invalid space specifier '%0' used; expected 'space' followed by an integer or something along those lines?

(Do we want to be kind and give users a fix-it for something like space 10 that removes the whitespace and recovers by pretending the user wrote space10 instead? Similar question about whitespace for register type.)

clang/include/clang/Parse/Parser.h
2821 ↗(On Diff #451282)

Spurious whitespace change that can be backed out.

clang/lib/Parse/ParseHLSL.cpp
223–224

This seems like a situation we should be asserting, right?

clang/lib/Sema/SemaDeclAttr.cpp
6952–6955

This code should not be necessary as the common handler should take care of it for you.

6962

This diagnostic is wrong, it should be firing: S.Diag(AL.getLoc(), diag::err_attribute_argument_type) << AL << AANT_ArgumentIdentifier;

Note, you are using ArgLoc but it's not initialized to a valid source location.

6974

Again, this is the wrong diagnostic here and it also uses an uninitialized source location.

6982

How do we get into this situation if the slot is mandatory?

7002

???

7022

Any reason not to do that now?

python3kgae marked 14 inline comments as done.

Remove optional flag for Slot.
Update error messages and doc.
Add test for empty resource attribute and space only resource attribute.

clang/include/clang/Basic/Attr.td
4051

Good catch.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11672–11673

Removed err_hlsl_unsupported_space_number.
The priority to add fix-it is very low compared to other things we need to add.

clang/lib/Sema/SemaDeclAttr.cpp
6982

It should not.
Fixed.

7022

Resource types like sampler/texture are ready yet.

Add fixit for case like space 1.

aaron.ballman added inline comments.Aug 12 2022, 5:03 AM
clang/include/clang/Basic/AttrDocs.td
6609–6610

These aren't really fully examples of how to use this properly. Can you add an actual example (it doesn't have to do anything particularly useful, the goal is largely to show off how a user would use this attribute)?

clang/lib/Parse/ParseHLSL.cpp
150–151

Any reason not to use ExpectAndConsume(tok::identifier, diag::err_expected) here instead?

176

Do we really mean insensitive here? (See comment below.)

clang/lib/Sema/SemaDeclAttr.cpp
6955–6965
7005

I'm surprised we want to be case insensitive here; we don't document that we are, but also, we're not case insensitive with the slot letters so it seems inconsistent. Is this part of the HLSL requirements?

(Note, I have no problems with doing a case insensitive check if the case sensitive check fails so we can give a better warning + fixit, if you thought that was useful.)

python3kgae marked an inline comment as done.

Limit to lower case.

python3kgae marked an inline comment as done.Aug 12 2022, 10:55 AM
python3kgae added inline comments.
clang/lib/Parse/ParseHLSL.cpp
150–151

Need to save the identifier string and loc for later use.
And the consume is done in ParseIdentifierLoc.

clang/lib/Sema/SemaDeclAttr.cpp
7005

Discussed with the team, we'll limit to lower case now.
If need for case insensitive for back-compat in the future, it is easy to enable it.

aaron.ballman added inline comments.Aug 12 2022, 11:25 AM
clang/include/clang/Basic/AttrDocs.td
6609–6610

Two things:

  • The example doesn't use valid RST, it should look more like what's done on AttrDocs:649 (using .. code-block:: c++)
  • I think the example is missing semicolons at the end of the declarations?
clang/lib/Parse/ParseHLSL.cpp
150–151

Ah, that's a good reason, thanks!

clang/lib/Sema/SemaDecl.cpp
2893–2894 ↗(On Diff #452232)

I don't see any tests covering this code path. How do you get such a redeclaration in HLSL?

clang/test/SemaHLSL/resource_binding_attr_error.hlsl
16

Isn't this a re-definition of a which would cause an error?

python3kgae marked 2 inline comments as done.

Remove mergeHLSLResourceBindingAttr since should not be able to set ResourceBinding more than once.

python3kgae added inline comments.Aug 12 2022, 4:14 PM
clang/lib/Sema/SemaDecl.cpp
2893–2894 ↗(On Diff #452232)

It should not.
I'll remove this code.

clang/test/SemaHLSL/resource_binding_attr_error.hlsl
16

The name for cbuffer is not used.
And no error should be reported for re-definition.

Discussed with the team, have to keep it like this for back-compat reason.

aaron.ballman accepted this revision.Aug 15 2022, 4:12 AM

LGTM, only nits left that can be fixed when landing (no need for additional review).

clang/include/clang/Basic/AttrDocs.td
6609–6612

RST is very picky about that whitespace.

clang/test/SemaHLSL/resource_binding_attr_error.hlsl
16

Wow.

Please either add a comment explaining why this bizarre situation gets no diagnostic, or change the identifiers used so nobody else gets confused by this when reading the test file.

This revision is now accepted and ready to land.Aug 15 2022, 4:12 AM
beanz added inline comments.Aug 15 2022, 10:02 AM
clang/test/SemaHLSL/resource_binding_attr_error.hlsl
16

cbuffer is really weird (and hopefully someday will disappear entirely). From a language perspective it behaves almost more like a named anonymous namespace than a datatype (yea, I know that sentence makes no sense).

The functionality that it provides is the ability to group a set of global constant variables so that the CPU-code can pack and provide those constants in known clumps.

One might ask why have them named at all? Which is a fair question... The names do get exposed via the CPU-side reflection API which supports querying bindings and the like. We _probably_ shouldn't have ever allowed duplicate names, but as long as they have unique "slot" bindings the names don't really matter.

cbuffers are important because we try to strip unused resources from shaders (because resource binding costs time). A cbuffer can either be striped entirely or not at all, which prevents individual binding elements from being stripped and simplifies shader calling code so the caller doesn't need to condition binding every individual resource element.

We have newer structures in the language which can handle all of the use cases for cbuffer, however there are still some cases (particularly targeting older versions of windows), where cbuffer is still the dominant technique.

Fix format in AttrDocs.td and avoid name conflict in test.

python3kgae added inline comments.Aug 15 2022, 11:54 AM
clang/test/SemaHLSL/resource_binding_attr_error.hlsl
16

Changed the names to avoid confusion.
More comment is added to https://reviews.llvm.org/D129883

Add Issue link for FIXME.

This revision was landed with ongoing or failed builds.Sep 22 2022, 11:51 AM
This revision was automatically updated to reflect the committed changes.