[PATCH] D123907: [HLSL] Add shader attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: beanz.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3978
+                            "compute", "raygeneration", "intersection",
+                            "anyhit", "closestHit", "miss", "callable", "mesh",
+                            "amplification"],
----------------
`anyhit` vs `closestHit` in terms of capitalization seems a bit surprising to me -- is that intentional?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6386-6388
+The ``shader`` attribute applies to HLSL shader entry functions to identify the
+ shader stage for the entry function.
+  }];
----------------
Please add enough documentation that a user has a chance to use this feature properly (and it's fine to link to other documentation for more details if there's canonical documentation elsewhere that we don't want to duplicate).


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11598-11599
 def err_hlsl_attribute_param_mismatch : Error<"%0 attribute parameters do not match the previous declaration">;
+def err_hlsl_invalid_attribute_argument : Error<
+  "%0 attribute argument not supported: %1">;
 
----------------
Almost every other attribute warns on this situation and then ignores the attribute; why should this be a hard error? (A warning allows other implementations to support more enumeration values without breaking the user's code in Clang.)


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6944-6947
+  if (AL.getNumArgs() != 1) {
+    S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << AL << 1;
+    return;
+  }
----------------
Hmm, doesn't the common attribute handling logic already deal with this case? (It does for other attribute syntaxes, but we have almost no attributes using the Microsoft `[]` syntax, so I wouldn't be surprised if we need to add that logic.)


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6961
+
+  // TODO: check function match the shader stage.
+
----------------
Are you planning to do this TODO as part of this patch, or is this a FIXME someone needs to handle later?


================
Comment at: clang/test/SemaHLSL/shader_attr.hlsl:20
+// expected-warning at +1 {{'shader' attribute only applies to global functions}}
+  [shader("vertex")]
+static void oops() {}
----------------
Formatting looks off here.

Also, the diagnostic text leaves a bit to be desired (this is more of a general problem with the `HLSLEntry` subject, so don't worry about it for your patch) -- this is a global function.


================
Comment at: clang/test/SemaHLSL/shader_attr.hlsl:25
+// expected-warning at +1 {{'shader' attribute only applies to global functions}}
+  [shader("vertex")]
+static void oops() {}
----------------
Formatting looks off here as well. I'm guessing clang-format doesn't do anything good with Microsoft-style attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123907