[PATCH] D122699: [HLSL] Add Semantic syntax, and SV_GroupIndex

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D122699#3422347 <https://reviews.llvm.org/D122699#3422347>, @beanz wrote:

> In D122699#3422298 <https://reviews.llvm.org/D122699#3422298>, @aaron.ballman wrote:
>
>> General question about the new syntax -- how does this work on field declarations of a record? e.g.,
>>
>>   struct S {
>>     int i : SV_GroupIndex;
>>   };
>>
>> (Please tell me the answer is: it doesn't, because bit-fields are a thing.)
>
> Oh @aaron.ballman… you’re such an optimist… Thankfully HLSL does’t support bitfields… or didn’t, until HLSL 2021…

I had a dream that I woke up on Monday and you said "btw, that was an April Fool's joke -- nobody is *that* bad at designing attribute syntax". :: sighs :: we'll cross that bridge when we get to it, I guess.

> This patch doesn’t add support for the semantic syntax anywhere other than on parameter declarations, which is enough to get us rolling to start. I was hoping to flesh out the rest of the places it is used over time so that we can figure out good ways to handle the bitfield ambiguity…

Yeah, we can definitely punt on the bit-field ambiguity. Frankly, I expect us to find more such issues. (This extension is nonconforming, so I think when we go to implement this part, we're going to have to be loud about diagnostics because this extension breaks conforming code.)

Mostly just nits at this point, so this is getting pretty close to being ready.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:6386
+  let Content = [{
+The ``SV_GroupIndex`` semantic when applied to an input parameter, specifies a
+data binding to map the group index to the specified parameter. This attribute
----------------
aaron.ballman wrote:
> Minor grammar nit
Might have missed this nit.


================
Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:47-48
 
     // Note TableGen depends on the order above.  Do not add or change the order
     // without adding related code to TableGen/ClangAttrEmitter.cpp.
     /// Context-sensitive version of a keyword attribute.
----------------
Should this comment now move down a bit?


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1600
+def err_expected_semantic_identifier : Error<
+"expected HLSL Semantic identifier">;
+def err_unknown_hlsl_semantic : Error<"unknown HLSL semantic %0">;
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6916
+  Triple Target = S.Context.getTargetInfo().getTriple();
+  if (Target.getEnvironment() != Triple::Compute) {
+    uint32_t Pipeline =
----------------
python3kgae wrote:
> It is OK to have SV_GroupIndex on a compute shader entry for library profile.
> 
Is there more to be done for this comment?


================
Comment at: clang/test/SemaHLSL/Semantics/semantic_parsing.hlsl:4-7
+void Entry(int GI : ) { }
+
+// expected-error at +1 {{unknown HLSL semantic 'SV_IWantAPony'}}
+void Pony(int GI : SV_IWantAPony) { }
----------------
This isn't really a Sema test because it's testing parsing behavior -- thoughts on adding `ParserHLSL` as a test directory (or do you expect so few parsing changes for this that it'd be a waste)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122699