[PATCH] D130033: [HLSL] Add resource binding attribute for HLSL.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:150
+                    [{isa<HLSLBufferDecl>(S)}],
+                    "global functions">;
 
----------------
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.


================
Comment at: clang/include/clang/Basic/Attr.td:4022
+  let LangOpts = [HLSL];
+  let Args = [StringArgument<"Slot", 1>, StringArgument<"Space", 1>];
+  let Documentation = [HLSLResourceBindingDocs];
----------------
Do you really intend `Slot` to be optional?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6545
+The resource binding attribute sets the virtual register and logical register space for a resource.
+Attribute spelling in HLSL is: ``register(slot [, space])``.
+``slot`` takes the format ``[type][number]``,
----------------
This documentation says `slot` is mandatory, hence the question above about the argument.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6556
+Register space is specified in the format ``space[number]`` and defaults to ``space0`` if omitted.
+
+The full documentation is available here: https://docs.microsoft.com/en-us/windows/win32/direct3d12/resource-binding-in-hlsl
----------------
I think it would help to have a working example here that shows both slot without space and slot with space.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1609
 def err_unknown_hlsl_semantic : Error<"unknown HLSL semantic %0">;
+
 def ext_hlsl_access_specifiers : ExtWarn<
----------------
Unintended whitespace change.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11650
 def err_hlsl_attribute_param_mismatch : Error<"%0 attribute parameters do not match the previous declaration">;
+def err_hlsl_empty_register_attr : Error<"register attribute cannot be empty">;
+def err_hlsl_unsupported_register_type : Error<"register type is unrecognized; expected resource class specifier 'b', 's', 't', 'u'">;
----------------
This diagnostic is not necessary; the common attribute handler already covers this. You're missing test coverage to verify that though.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11651
+def err_hlsl_empty_register_attr : Error<"register attribute cannot be empty">;
+def err_hlsl_unsupported_register_type : Error<"register type is unrecognized; expected resource class specifier 'b', 's', 't', 'u'">;
+def err_hlsl_unsupported_register_number : Error<"register number should be an integer">;
----------------
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'`?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11653-11654
+def err_hlsl_unsupported_register_number : Error<"register number should be an integer">;
+def err_hlsl_expected_space : Error<"expected space argument specified as 'spaceX', where X is an integer">;
+def err_hlsl_unsupported_space_number : Error<"space number should be an integer">;
 
----------------
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.)


================
Comment at: clang/include/clang/Parse/Parser.h:2821
                           SourceLocation *EndLoc = nullptr);
+
   Decl *ParseHLSLBuffer(SourceLocation &DeclEnd,
----------------
Spurious whitespace change that can be backed out.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:121-122
+  } break;
+  default:
+    break;
+  }
----------------
This seems like a situation we should be asserting, right?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6939-6942
+  if (AL.getNumArgs() == 0) {
+    S.Diag(AL.getLoc(), diag::err_hlsl_empty_register_attr);
+    return;
+  }
----------------
This code should not be necessary as the common handler should take care of it for you.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6949
+  if (!AL.isArgIdent(0)) {
+    S.Diag(ArgLoc, diag::err_hlsl_unsupported_register_type);
+    return;
----------------
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.



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6961
+    if (!AL.isArgIdent(1)) {
+      S.Diag(SpaceArgLoc, diag::err_hlsl_expected_space);
+      return;
----------------
Again, this is the wrong diagnostic here and it also uses an uninitialized source location.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6969
+  } else {
+    if (Str.startswith_insensitive("space")) {
+      Space = Str;
----------------
How do we get into this situation if the slot is mandatory?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6989
+    }
+    "register number should be an integral numeric string";
+    StringRef SlotNum = Slot.substr(1);
----------------
???


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7009
+
+  // FIXME: check reg type match decl.
+  HLSLResourceBindingAttr *NewAttr =
----------------
Any reason not to do that now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130033