[HLSL] Align implementation of cbuffer declarations with spec

[bogner]

Consider https://hlsl.godbolt.org/z/x6sT7baY8:

// -T cs_6_4
cbuffer Bar {
  namespace NS {
    float i;
  }
}
RWStructuredBuffer<float> Out;

[numthreads(1,1,1)]
void main() {
  Out[0] = NS::i;
}

As per microsoft/hlsl-specs#671, namespaces nested inside cbuffer decls are allowed. We shouldn't error on them.

The fix should be as simple as updating validateDeclsInsideHLSLBuffer to stop explicitly disallowing them.

[llvm-beanz]

Please hold on this. I'm not sure this is the right decision. We have an active proposal to change this behavior in HLSL 202x (see: cbuffer contexts).

FXC treated cbuffer declarations semantically like a weird pragma-esque region but grammatically as a declaration context. DXC has bugs and kinda does its own thing. We've proposed simplifying this for HLSL 202x to be more intuitive to users, and we should bottom out on that discussion before making changes here.

[bogner]

The code as we have it today is the worst of both worlds, so we should really do something here. As it is now, DeclContext::isTransparentContext() returns true for HLSLBufferDecl but we explicitly disallow namespaces (and cbuffers) in ParseHLSL.cpp in validateDeclsInsideHLSLBuffer. This function explicitly allows record decls, function decls, and other constructs that the currently under consideration proposal aims to ban.

We need to make changes to what we consider valid decls in a buffer whether or not we accept that proposal, as the current state is incompatible with DXC and FXC in a very unintentional way. I think the most defensible short term solution is the following, and we can revisit once we've gotten further along with the cbuffer contexts proposal, which would need further code changes in any case.

diff --git a/clang/lib/Parse/ParseHLSL.cpp b/clang/lib/Parse/ParseHLSL.cpp
index 51f2aef86964..7c8d4e0805c0 100644
--- a/clang/lib/Parse/ParseHLSL.cpp
+++ b/clang/lib/Parse/ParseHLSL.cpp
@@ -30,11 +30,12 @@ static bool validateDeclsInsideHLSLBuffer(Parser::DeclGroupPtrTy DG,
   // Only allow function, variable, record, and empty decls inside HLSLBuffer.
   for (DeclGroupRef::iterator I = Decls.begin(), E = Decls.end(); I != E; ++I) {
     Decl *D = *I;
-    if (isa<CXXRecordDecl, RecordDecl, FunctionDecl, VarDecl, EmptyDecl>(D))
+    if (isa<CXXRecordDecl, RecordDecl, FunctionDecl, VarDecl, EmptyDecl,
+            NamespaceDecl>(D))
       continue;

-    // FIXME: support nested HLSLBuffer and namespace inside HLSLBuffer.
-    if (isa<HLSLBufferDecl, NamespaceDecl>(D)) {
+    // TODO: Either support nested buffers or explicitly ban them in HLSL 202x
+    if (isa<HLSLBufferDecl>(D)) {
       P.Diag(D->getLocation(), diag::err_invalid_declaration_in_hlsl_buffer)
           << IsCBuffer;
       IsValid = false;

[damyanp]

@llvm-beanz to make sure this is addressed in the updates he's working on now.

[llvm-beanz]

The patch posted by @bogner above doesn't address the complexities of nesting cbuffers and once you allow a namespace in a cbuffer and a cbuffer in a namespace the nesting becomes potentially infinite.

The current proposal is to disallow nested cbuffers and namespaces in cbuffers, but to continue to allow all declarations that are valid at block scope in a cbuffer. We may further restrict this in future language versions.

The current spec PR is microsoft/hlsl-specs#685.