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

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

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.



================
Comment at: clang/include/clang/Basic/Attr.td:4018
+                           ["SRV", "UAV", "CBuffer", "Sampler"]
+                           >, DefaultIntArgument<"ID", 0>, DefaultIntArgument<"Space", 0>];
+  let Documentation = [HLSLResourceBindingDocs];
----------------
`ID` seems like the wrong name here as it doesn't really convey the meaning of the value. Maybe `register` or `slot`?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6516
+  let Content = [{
+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).
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:6517
+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
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:6518
+Format is ''register(ID,  space)'' like register(t3,  space1).
+ID must be start with
+t for shader resource views (SRV),
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:6524
+
+Register space is default to space0.
+
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1607
 def err_unknown_hlsl_semantic : Error<"unknown HLSL semantic %0">;
+def err_hlsl_unsupported_register_type : Error<"register type is unsupported - available types are 'b', 's', 't', 'u'">;
+def err_hlsl_unsupported_register_number : Error<"register number should be an integral numeric string">;
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1608
+def err_hlsl_unsupported_register_type : Error<"register type is unsupported - available types are 'b', 's', 't', 'u'">;
+def err_hlsl_unsupported_register_number : Error<"register number should be an integral numeric string">;
+def err_hlsl_expected_space : Error<"expected space definition with syntax 'spaceX', where X is an integral value">;
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1609
+def err_hlsl_unsupported_register_number : Error<"register number should be an integral numeric string">;
+def err_hlsl_expected_space : Error<"expected space definition with syntax 'spaceX', where X is an integral value">;
+def err_hlsl_unsupported_space_number : Error<"space number should be an integral numeric string">;
----------------



================
Comment at: clang/include/clang/Parse/Parser.h:2821
+
+  void MaybeParseHLSLResourceBinding(ParsedAttributes &Attrs,
+                                     SourceLocation *EndLoc = nullptr) {
----------------
I don't think this should be fully separate from parsing other HLSL semantics.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:96
+  auto KwLoc = Tok.getLocation();
+  ConsumeToken(); // consume kw_register.
+
----------------
register isn't a kw.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130033