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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

There's a whole ton of test coverage missing from this (no actual sema tests, no AST dumping tests for text or JSON) not to mention a ton of situational tests that are missing. For example, can you put one of these declarations inside of a struct (so it's a member declaration)? How about declaring one as a local variable? Tests that it properly diagnoses use of a template, as in `template <typename Ty> cbuffer a { Ty f; };`? Are there restrictions on what can be declared within one of these buffers (can I declare an inline function in one?)? All those sorts of situations and more should be tested explicitly.



================
Comment at: clang/include/clang/AST/Decl.h:4675
+/// HLSLBufferDecl - Represent a cbuffer or tbuffer declaration.
+class HLSLBufferDecl : public NamedDecl, public DeclContext {
+  /// LBraceLoc - The ending location of the source range.
----------------
Are these redeclarable declarations? I assume not based on previous discussions, so mostly just double checking.


================
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);
----------------
Speculative question: would it make more sense to add `CreateCBuffer` and `CreateTBuffer` static methods rather than use a `bool` parameter?


================
Comment at: clang/include/clang/AST/Decl.h:4696
+
+  virtual SourceRange getSourceRange() const LLVM_READONLY {
+    return SourceRange(getLocStart(), RBraceLoc);
----------------
Nope. :-D


================
Comment at: clang/include/clang/AST/Decl.h:4699-4700
+  }
+  const char *getDeclKindName() const;
+  SourceLocation getLocStart() const LLVM_READONLY { return KwLoc; }
+  SourceLocation getRBraceLoc() const { return RBraceLoc; }
----------------
Unused and unimplemented.


================
Comment at: clang/lib/AST/Decl.cpp:5228-5230
+  if (DC != LexicalParent) {
+    Result->setLexicalDeclContext(LexicalParent);
+  }
----------------
So semantically these are all in the global namespace no matter where they're spelled? e.g., if you put one inside of a namespace... it's still globally accessible?


================
Comment at: clang/lib/AST/DeclBase.cpp:1197-1198
 
-  return getDeclKind() == Decl::LinkageSpec || getDeclKind() == Decl::Export;
+  return getDeclKind() == Decl::LinkageSpec || getDeclKind() == Decl::Export ||
+         getDeclKind() == Decl::HLSLBuffer;
 }
----------------



================
Comment at: clang/lib/AST/DeclPrinter.cpp:466-472
     } else if (isa<NamespaceDecl>(*D) || isa<LinkageSpecDecl>(*D) ||
              isa<ObjCImplementationDecl>(*D) ||
              isa<ObjCInterfaceDecl>(*D) ||
              isa<ObjCProtocolDecl>(*D) ||
              isa<ObjCCategoryImplDecl>(*D) ||
-             isa<ObjCCategoryDecl>(*D))
+             isa<ObjCCategoryDecl>(*D) ||
+             isa<HLSLBufferDecl>(*D))
----------------
Let's make this better while we're touching it.


================
Comment at: clang/lib/AST/DeclPrinter.cpp:1664-1668
+  if (D->isCBuffer()) {
+    Out << "cbuffer ";
+  } else {
+    Out << "tbuffer ";
+  }
----------------



================
Comment at: clang/lib/AST/TextNodeDumper.cpp:2390
+
+void TextNodeDumper::VisitHLSLBufferDecl(const HLSLBufferDecl *D) {
+  if (D->isCBuffer())
----------------
You're missing similar changes for the `JSONNodeDumper`.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:1793
+  case tok::kw_tbuffer:
+    SingleDecl = ParseHLSLBuffer(DeclEnd);
+    break;
----------------
So these can form a declaration group? e.g.,
```
cbuffer a { int b; }, b { float f; };
```
I don't see any test coverage for this situation if that is supported. If it's not supported, I think this should be a `return` statement instead.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:48-52
+  while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
+    ParsedAttributes Attrs(AttrFactory);
+    // FIXME: support attribute on constants inside cbuffer/tbuffer.
+    ParseExternalDeclaration(Attrs);
+  }
----------------
This looks like it's going to have some pretty bad error recovery behavior if something is amiss:
```
cbuffer a {
  oh no!
  this isn't even valid HLSL code
  despite seeming totally reasonable
  once you understand that HLSL
  is so flaming weird.
};
```


================
Comment at: clang/lib/Sema/SemaHLSL.cpp:27
+
+  ActOnDocumentableDecl(Result);
+
----------------
I don't see any tests for this.


================
Comment at: clang/lib/Sema/SemaHLSL.cpp:33
+void Sema::ActOnFinishHLSLBuffer(Decl *Dcl, SourceLocation RBrace) {
+  HLSLBufferDecl *BufDecl = dyn_cast_or_null<HLSLBufferDecl>(Dcl);
+  assert(BufDecl && "Invalid parameter, expected HLSLBufferDecl");
----------------
(The interface was deprecated recently and this is the replacement API.)


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:877
+Decl *TemplateDeclInstantiator::VisitHLSLBufferDecl(HLSLBufferDecl *Decl) {
+  llvm_unreachable("HLSL buffer declarations cannot be instantiated");
+}
----------------
Is this actually unreachable?


================
Comment at: clang/test/AST/HLSL/cbuffer_tbuffer.hlsl:7
+  float a;
+}
+
----------------
Something's amiss -- this declaration doesn't have a semicolon, is that expected? Because the SemaHLSL tests do have terminating semicolons and no diagnostics about it being superfluous.


================
Comment at: clang/test/SemaHLSL/cb_error.hlsl:3-21
+// expected-error at +2 {{expected identifier}}
+// expected-error at +1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error at +1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error at +1 {{expected unqualified-id}}
+int cbuffer;
----------------
None of these are Sema tests, they're all parsing tests. I think the content should be moved to the parsing test directory and sema tests should be added. For example, we should clearly test using the same identifiers for these buffers and comment about why that's valid and not an error. We should also have tests that demonstrate name lookup behavior, e.g., what happens with this:
```
cbuffer a {
  int x;
};

int main() {
  return sizeof(a);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129883