[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for the update, this is heading in a good direction! I think there's still a lot of test coverage missing. Consider:

  namespace Name {
  cbuffer a {
    int x;
  }
  }
  
  struct S {
    cbuffer what {
      int y;
    }
  };
  
  void func() {
    tbuffer derp {
      int z;
    }
  
    decltype(derp) another {
      int a;
    }
  }



================
Comment at: clang/include/clang/AST/Decl.h:4690-4693
+  static HLSLBufferDecl *Create(ASTContext &C, DeclContext *LexicalParent,
+                                bool CBuffer, SourceLocation KwLoc,
+                                IdentifierInfo *ID, SourceLocation IDLoc,
+                                SourceLocation LBrace);
----------------
aaron.ballman wrote:
> Speculative question: would it make more sense to add `CreateCBuffer` and `CreateTBuffer` static methods rather than use a `bool` parameter?
I'm still on the fence about this. On the one hand, being explicit is far better than hoping the reader understand the bool argument at the call site. On the other hand, using the bool parameter means less branching at the call sites. So I'm leaning towards leaving this as-is, but if @beanz has other opinions, I don't yet have a strong feeling.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1608
   "expected HLSL Semantic identifier">;
+def err_invalid_declaration_in_hlsl_buffer : Error<"invalid declaration inside cbuffer/tbuffer">;
 def err_unknown_hlsl_semantic : Error<"unknown HLSL semantic %0">;
----------------
You know what kind of buffer it's within so we can be more precise.


================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:904
+  VisitNamedDecl(D);
+  JOS.attribute("buffer_kind", D->isCBuffer() ? "cbuffer" : "tbuffer");
+}
----------------
Matches the naming style for most everything else in the file.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:20-21
 
+Decl *Parser::ParseHLSLBuffer(SourceLocation &DeclEnd,
+                              SourceLocation InlineLoc) {
+  assert((Tok.is(tok::kw_cbuffer) || Tok.is(tok::kw_tbuffer)) &&
----------------
Parameter is unused, I presume that's intentional rather than accidental?


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:25
+  bool IsCBuffer = Tok.is(tok::kw_cbuffer);
+  SourceLocation BufferLoc = ConsumeToken(); // eat the 'cbuffer or tbuffer'.
+
----------------



================
Comment at: clang/lib/Parse/ParseHLSL.cpp:33
+  IdentifierInfo *Identifier = Tok.getIdentifierInfo();
+  SourceLocation IdentifierLoc = ConsumeToken(); // consume identifier
+
----------------



================
Comment at: clang/lib/Parse/ParseHLSL.cpp:50-54
+    // FIXME: support attribute on constants inside cbuffer/tbuffer.
+    ParsedAttributes Attrs(AttrFactory);
+    ParsedAttributes DeclSpecAttrs(AttrFactory);
+
+    if (PP.isCodeCompletionReached()) {
----------------



================
Comment at: clang/lib/Parse/ParseHLSL.cpp:59
+
+    switch (Tok.getKind()) {
+    case tok::kw_namespace:
----------------
The approach of using a switch and handling individual keywords specially strikes me as being quite fragile. I think a better approach is to split this loop out into its own function and model it (at least somewhat) after `ParseStructUnionBody()`.

The current approach worries me because I have no idea why there's a giant block of invalid things or what should be added to that as we add new declarations to the compiler (and certainly nobody is going to think to come update this function when adding support for new kinds of declarations.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:64-65
+      T.skipToEnd();
+      return nullptr;
+      break;
+    case tok::kw_cbuffer:
----------------



================
Comment at: clang/lib/Parse/ParseHLSL.cpp:71-74
+      return nullptr;
+
+      [[fallthrough]];
+    case tok::annot_pragma_vis:
----------------
There's no way to reach this, so we shouldn't need the `[[fallthrough]]` here, right?


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:103-104
+    case tok::kw_export:
+    case tok::kw_using:
+    case tok::kw_typedef:
+    case tok::kw_template:
----------------
Why are type aliases prohibited?


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:106-107
+    case tok::kw_template:
+    case tok::kw_static_assert:
+    case tok::kw__Static_assert:
+    case tok::kw_inline:
----------------
Why are static assertions prohibited?


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:122-123
+    case tok::kw_static:
+      // Parse (then ignore) 'static' prior to a template instantiation. This
+      // is a GCC extension that we intentionally do not support.
+      if (getLangOpts().CPlusPlus && NextToken().is(tok::kw_template)) {
----------------
1) GCC supports HLSL?

2) This seems wrong because it will result in:
```
static template <typename Ty> Ty Val{}; // Prohibited
static void func(); // Allowed
inline void other(); // Prohibited
static inline void haha(); // Allowed
```
As mentioned above, I think we should use general parsing mechanisms here and then look at the AST node generated to see whether it's an allowed one or not.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:134
+      }
+      ParseSimpleDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+                             DeclSpecAttrs, true, nullptr, &Loc);
----------------
aaron.ballman wrote:
> You're parsing things and then dropping them on the floor?
The declarator context looks wrong to me -- I don't see anything prohibiting the user from doing:
```
namespace N {
cbuffer {
  ...
}
}
```



================
Comment at: clang/lib/Parse/ParseHLSL.cpp:134-135
+      }
+      ParseSimpleDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+                             DeclSpecAttrs, true, nullptr, &Loc);
+      break;
----------------
You're parsing things and then dropping them on the floor?


================
Comment at: clang/test/ParserHLSL/invalid_inside_cb.hlsl:20-24
+    -
+}
+cbuffer A {
+// expected-error at +1 {{invalid declaration inside cbuffer/tbuffer}}
+    +
----------------
These aren't declarations or even really close (maybe if you squint and think about Objective-C?) so the diagnostic here is really weird. I'd expect to be some sort of syntax error because we have no idea what to parse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129883