Code review feedback

- create common function isInvalidConstantBufferLeafElementType
- create layout structs in the same context as the original struct
- fix lookup to scan just the first non-transparent context
- add test case with namespaces

Author: hekota

clang/lib/Sema/SemaHLSL.cpp

@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/Type.h"
@@ -46,8 +47,8 @@
 using namespace clang;
 using RegisterType = HLSLResourceBindingAttr::RegisterType;
 
-static CXXRecordDecl *createHostLayoutStruct(Sema &S, CXXRecordDecl *StructDecl,
-                                             HLSLBufferDecl *BufDecl);
+static CXXRecordDecl *createHostLayoutStruct(Sema &S,
+                                             CXXRecordDecl *StructDecl);
 
 static RegisterType getRegisterType(ResourceClass RC) {
   switch (RC) {
@@ -268,6 +269,30 @@ static bool isZeroSizedArray(const ConstantArrayType *CAT) {
   return CAT != nullptr;
 }
 
+// Returns true if the record type is an HLSL resource class
+static bool isResourceRecordType(const Type *Ty) {
+  return HLSLAttributedResourceType::findHandleTypeOnResource(Ty) != nullptr;
+}
+
+// Returns true if the type is a leaf element type that is not valid to be included
+// in HLSL Buffer, such as a resource class, empty struct, zero-sized array,
+// or a builtin intangible type.
+// Returns false it is a valid leaf element type or if it is a record type that
+// needs to be inspected further.
+static bool isInvalidConstantBufferLeafElementType(const Type *Ty) {
+  if (Ty->isRecordType()) {
+    if (isResourceRecordType(Ty) || Ty->getAsCXXRecordDecl()->isEmpty())
+      return true;
+    return false;
+  }
+  if (Ty->isConstantArrayType() &&
+      isZeroSizedArray(cast<ConstantArrayType>(Ty)))
+    return true;
+  if (Ty->isHLSLBuiltinIntangibleType())
+    return true;
+  return false;
+}
+
 // Returns true if the struct contains at least one element that prevents it
 // from being included inside HLSL Buffer as is, such as an intangible type,
 // empty struct, or zero-sized array. If it does, a new implicit layout struct
@@ -279,13 +304,11 @@ static bool requiresImplicitBufferLayoutStructure(const CXXRecordDecl *RD) {
   // check fields
   for (const FieldDecl *Field : RD->fields()) {
     QualType Ty = Field->getType();
-    if (Ty->isRecordType()) {
-      if (requiresImplicitBufferLayoutStructure(Ty->getAsCXXRecordDecl()))
-        return true;
-    } else if (Ty->isConstantArrayType()) {
-      if (isZeroSizedArray(cast<ConstantArrayType>(Ty)))
-        return true;
-    }
+    if (isInvalidConstantBufferLeafElementType(Ty.getTypePtr()))
+      return true;
+    if (Ty->isRecordType() &&
+        requiresImplicitBufferLayoutStructure(Ty->getAsCXXRecordDecl()))
+      return true;
   }
   // check bases
   for (const CXXBaseSpecifier &Base : RD->bases())
@@ -295,25 +318,28 @@ static bool requiresImplicitBufferLayoutStructure(const CXXRecordDecl *RD) {
   return false;
 }
 
-static CXXRecordDecl *findRecordDecl(Sema &S, IdentifierInfo *II,
-                                     DeclContext *DC) {
-  DeclarationNameInfo NameInfo =
-      DeclarationNameInfo(DeclarationName(II), SourceLocation());
-  LookupResult R(S, NameInfo, Sema::LookupTagName);
-  S.LookupName(R, S.getScopeForContext(DC));
-  if (R.isSingleResult())
-    return R.getAsSingle<CXXRecordDecl>();
-  return nullptr;
+static CXXRecordDecl *findRecordDeclInContext(IdentifierInfo *II,
+                                              DeclContext *DC) {
+  CXXRecordDecl *RD = nullptr;
+  for (NamedDecl *Decl :
+       DC->getNonTransparentContext()->lookup(DeclarationName(II))) {
+    if (CXXRecordDecl *FoundRD = dyn_cast<CXXRecordDecl>(Decl)) {
+      assert(RD == nullptr &&
+             "there should be at most 1 record by a given name in a scope");
+      RD = FoundRD;
+    }
+  }
+  return RD;
 }
 
 // Creates a name for buffer layout struct using the provide name base.
 // If the name must be unique (not previously defined), a suffix is added
 // until a unique name is found.
-static IdentifierInfo *getHostLayoutStructName(Sema &S,
-                                               IdentifierInfo *NameBaseII,
-                                               bool MustBeUnique,
-                                               DeclContext *DC) {
+static IdentifierInfo *getHostLayoutStructName(Sema &S, NamedDecl *BaseDecl,
+                                               bool MustBeUnique) {
   ASTContext &AST = S.getASTContext();
+
+  IdentifierInfo *NameBaseII = BaseDecl->getIdentifier();
   StringRef NameBase;
   if (NameBaseII) {
     NameBase = NameBaseII->getName();
@@ -333,39 +359,31 @@ static IdentifierInfo *getHostLayoutStructName(Sema &S,
     if (suffix != 0)
       II = &AST.Idents.get((Name + "_" + Twine(suffix)).str(),
                            tok::TokenKind::identifier);
-    if (!findRecordDecl(S, II, DC))
+    if (!findRecordDeclInContext(II, BaseDecl->getDeclContext()))
       return II;
     // declaration with that name already exists - increment suffix and try
     // again until unique name is found
     suffix++;
   };
 }
 
-// Returns true if the record type is an HLSL resource class
-static bool isResourceRecordType(const Type *Ty) {
-  return HLSLAttributedResourceType::findHandleTypeOnResource(Ty) != nullptr;
-}
-
 // Creates a field declaration of given name and type for HLSL buffer layout
 // struct. Returns nullptr if the type cannot be use in HLSL Buffer layout.
 static FieldDecl *createFieldForHostLayoutStruct(Sema &S, const Type *Ty,
                                                  IdentifierInfo *II,
-                                                 CXXRecordDecl *LayoutStruct,
-                                                 HLSLBufferDecl *BufDecl) {
+                                                 CXXRecordDecl *LayoutStruct) {
+  if (isInvalidConstantBufferLeafElementType(Ty))
+    return nullptr;
+
   if (Ty->isRecordType()) {
-    if (isResourceRecordType(Ty))
-      return nullptr;
     CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
     if (requiresImplicitBufferLayoutStructure(RD)) {
-      RD = createHostLayoutStruct(S, RD, BufDecl);
+      RD = createHostLayoutStruct(S, RD);
       if (!RD)
         return nullptr;
       Ty = RD->getTypeForDecl();
     }
   }
-  if (Ty->isConstantArrayType() &&
-      isZeroSizedArray(cast<ConstantArrayType>(Ty)))
-    return nullptr;
 
   QualType QT = QualType(Ty, 0);
   ASTContext &AST = S.getASTContext();
@@ -384,23 +402,21 @@ static FieldDecl *createFieldForHostLayoutStruct(Sema &S, const Type *Ty,
 // - empty structs
 // - zero-sized arrays
 // Returns nullptr if the resulting layout struct would be empty.
-static CXXRecordDecl *createHostLayoutStruct(Sema &S, CXXRecordDecl *StructDecl,
-                                             HLSLBufferDecl *BufDecl) {
+static CXXRecordDecl *createHostLayoutStruct(Sema &S,
+                                             CXXRecordDecl *StructDecl) {
   assert(requiresImplicitBufferLayoutStructure(StructDecl) &&
          "struct is already HLSL buffer compatible");
 
   ASTContext &AST = S.getASTContext();
   DeclContext *DC = StructDecl->getDeclContext();
-  IdentifierInfo *II = getHostLayoutStructName(
-      S, StructDecl->getIdentifier(), false, BufDecl->getDeclContext());
+  IdentifierInfo *II = getHostLayoutStructName(S, StructDecl, false);
 
   // reuse existing if the layout struct if it already exists
-  if (CXXRecordDecl *RD = findRecordDecl(S, II, DC))
+  if (CXXRecordDecl *RD = findRecordDeclInContext(II, DC))
     return RD;
 
-  CXXRecordDecl *LS =
-      CXXRecordDecl::Create(AST, TagDecl::TagKind::Class, BufDecl,
-                            SourceLocation(), SourceLocation(), II);
+  CXXRecordDecl *LS = CXXRecordDecl::Create(
+      AST, TagDecl::TagKind::Class, DC, SourceLocation(), SourceLocation(), II);
   LS->setImplicit(true);
   LS->startDefinition();
 
@@ -410,7 +426,7 @@ static CXXRecordDecl *createHostLayoutStruct(Sema &S, CXXRecordDecl *StructDecl,
     CXXBaseSpecifier Base = *StructDecl->bases_begin();
     CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
     if (requiresImplicitBufferLayoutStructure(BaseDecl)) {
-      BaseDecl = createHostLayoutStruct(S, BaseDecl, BufDecl);
+      BaseDecl = createHostLayoutStruct(S, BaseDecl);
       if (BaseDecl) {
         TypeSourceInfo *TSI = AST.getTrivialTypeSourceInfo(
             QualType(BaseDecl->getTypeForDecl(), 0));
@@ -427,15 +443,16 @@ static CXXRecordDecl *createHostLayoutStruct(Sema &S, CXXRecordDecl *StructDecl,
   // filter struct fields
   for (const FieldDecl *FD : StructDecl->fields()) {
     const Type *Ty = FD->getType()->getUnqualifiedDesugaredType();
-    if (FieldDecl *NewFD = createFieldForHostLayoutStruct(
-            S, Ty, FD->getIdentifier(), LS, BufDecl))
+    if (FieldDecl *NewFD =
+            createFieldForHostLayoutStruct(S, Ty, FD->getIdentifier(), LS))
       LS->addDecl(NewFD);
   }
   LS->completeDefinition();
 
   if (LS->field_empty() && LS->getNumBases() == 0)
     return nullptr;
-  BufDecl->addDecl(LS);
+
+  DC->addDecl(LS);
   return LS;
 }
 
@@ -449,8 +466,7 @@ static CXXRecordDecl *createHostLayoutStruct(Sema &S, CXXRecordDecl *StructDecl,
 // The layour struct will be added to the HLSLBufferDecl declarations.
 void createHostLayoutStructForBuffer(Sema &S, HLSLBufferDecl *BufDecl) {
   ASTContext &AST = S.getASTContext();
-  IdentifierInfo *II = getHostLayoutStructName(S, BufDecl->getIdentifier(),
-                                               true, BufDecl->getDeclContext());
+  IdentifierInfo *II = getHostLayoutStructName(S, BufDecl, true);
 
   CXXRecordDecl *LS =
       CXXRecordDecl::Create(AST, TagDecl::TagKind::Class, BufDecl,
@@ -463,8 +479,8 @@ void createHostLayoutStructForBuffer(Sema &S, HLSLBufferDecl *BufDecl) {
     if (!VD || VD->getStorageClass() == SC_Static)
       continue;
     const Type *Ty = VD->getType()->getUnqualifiedDesugaredType();
-    if (FieldDecl *FD = createFieldForHostLayoutStruct(
-            S, Ty, VD->getIdentifier(), LS, BufDecl))
+    if (FieldDecl *FD =
+            createFieldForHostLayoutStruct(S, Ty, VD->getIdentifier(), LS))
       LS->addDecl(FD);
   }
   LS->completeDefinition();

clang/test/AST/HLSL/cbuffer.hlsl

@@ -87,54 +87,55 @@ cbuffer CB {
   B s2;
   // CHECK: VarDecl {{.*}} col:12 s3 'CTypedef':'C
   CTypedef s3;
-  // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_B definition
-  // CHECK: FieldDecl {{.*}} a 'float'
   // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_CB_2 definition
   // CHECK: FieldDecl {{.*}} s1 'A'
   // CHECK: FieldDecl {{.*}} s2 '__layout_B'
 }
+// CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_B definition
+// CHECK: FieldDecl {{.*}} a 'float'
+
 _Static_assert(__builtin_hlsl_is_scalarized_layout_compatible(OneFloat, __layout_B), "");
 _Static_assert(__builtin_hlsl_is_scalarized_layout_compatible(TwoFloats, __layout_CB_2), "");
 
 // check that layout struct is created for D because of its base struct
-// CHECK: HLSLBufferDecl {{.*}} line:103:9 cbuffer CB
+// CHECK: HLSLBufferDecl {{.*}} line:104:9 cbuffer CB
 // CHECK: HLSLResourceClassAttr {{.*}} Implicit CBuffer
 // CHECK: HLSLResourceAttr {{.*}} Implicit CBuffer
 cbuffer CB {
   // CHECK: VarDecl {{.*}} s4 'D'
   D s4;
-  // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_D definition
-  // CHECK: public '__layout_B'
-  // CHECK: FieldDecl {{.*}} b 'float'
   // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_CB_3 definition
   // CHECK: FieldDecl {{.*}} s4 '__layout_D'
 }
+  // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_D definition
+  // CHECK: public '__layout_B'
+  // CHECK: FieldDecl {{.*}} b 'float'
 _Static_assert(__builtin_hlsl_is_scalarized_layout_compatible(TwoFloats, __layout_D), "");
 _Static_assert(__builtin_hlsl_is_scalarized_layout_compatible(TwoFloats, __layout_CB_3), "");
 
 // check that layout struct is created for E because because its base struct
 // is empty and should be eliminated, and BTypedef should reuse the previously
 // defined '__layout_B' 
-// CHECK: HLSLBufferDecl {{.*}} line:121:9 cbuffer CB
+// CHECK: HLSLBufferDecl {{.*}} line:122:9 cbuffer CB
 // CHECK: HLSLResourceClassAttr {{.*}} Implicit CBuffer
 // CHECK: HLSLResourceAttr {{.*}} Implicit CBuffer
 cbuffer CB {
   // CHECK: VarDecl {{.*}}  s5 'E'
   E s5;
   // CHECK: VarDecl {{.*}} s6 'BTypedef':'B'
   BTypedef s6;
-  // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_E definition
-  // CHECK: FieldDecl {{.*}} c 'float'
-  // CHECK-NOT: CXXRecordDecl {{.*}} class __layout_B definition
   // CHECK: CXXRecordDecl {{.*}}  implicit referenced class __layout_CB_4 definition
   // CHECK: FieldDecl {{.*}} s5 '__layout_E'
   // CHECK: FieldDecl {{.*}} s6 '__layout_B'
 }
+  // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_E definition
+  // CHECK: FieldDecl {{.*}} c 'float'
+  // CHECK-NOT: CXXRecordDecl {{.*}} class __layout_B definition
 _Static_assert(__builtin_hlsl_is_scalarized_layout_compatible(OneFloat, __layout_E), "");
 _Static_assert(__builtin_hlsl_is_scalarized_layout_compatible(TwoFloats, __layout_CB_4), "");
 
 // check that this produces empty layout struct
-// CHECK: HLSLBufferDecl {{.*}} line:140:9 cbuffer CB
+// CHECK: HLSLBufferDecl {{.*}} line:141:9 cbuffer CB
 // CHECK: HLSLResourceClassAttr {{.*}} Implicit CBuffer
 // CHECK: HLSLResourceAttr {{.*}} Implicit CBuffer
 cbuffer CB {
@@ -153,22 +154,22 @@ cbuffer CB {
 }
 
 // check host layout struct with compatible base struct
-// CHECK: HLSLBufferDecl {{.*}} line:159:9 cbuffer CB
+// CHECK: HLSLBufferDecl {{.*}} line:160:9 cbuffer CB
 // CHECK: HLSLResourceClassAttr {{.*}} Implicit CBuffer
 // CHECK: HLSLResourceAttr {{.*}} Implicit CBuffer
 cbuffer CB {
   // CHECK: VarDecl {{.*}} s8 'F'
   F s8;
-  // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_F definition
-  // CHECK: public 'A'
   // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_CB_6 definition
   // CHECK: FieldDecl {{.*}} s8 '__layout_F'
 }
+  // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_F definition
+  // CHECK: public 'A'
 _Static_assert(__builtin_hlsl_is_scalarized_layout_compatible(OneFloat, __layout_F), "");
 _Static_assert(__builtin_hlsl_is_scalarized_layout_compatible(OneFloat, __layout_CB_6), "");
 
 // anonymous structs
-// CHECK: HLSLBufferDecl {{.*}} line:174:9 cbuffer CB
+// CHECK: HLSLBufferDecl {{.*}} line:175:9 cbuffer CB
 // CHECK: HLSLResourceClassAttr {{.*}} Implicit CBuffer
 // CHECK: HLSLResourceAttr {{.*}} Implicit CBuffer
 cbuffer CB {
@@ -181,15 +182,15 @@ cbuffer CB {
     // CHECK: FieldDecl {{.*}} f 'RWBuffer<float>':'hlsl::RWBuffer<float>'
     RWBuffer<float> f;
   } s9;
-  // CHECK: VarDecl {{.*}} s9 'struct (unnamed struct at {{.*}}cbuffer.hlsl:176:3
+  // CHECK: VarDecl {{.*}} s9 'struct (unnamed struct at {{.*}}cbuffer.hlsl:177:3
   // CHECK: CXXRecordDecl {{.*}} struct definition
   struct {
     // CHECK: FieldDecl {{.*}} g 'float'
     float g;
     // CHECK: FieldDecl {{.*}} f 'RWBuffer<float>':'hlsl::RWBuffer<float>'
     RWBuffer<float> f;
   } s10;
-  // CHECK: VarDecl {{.*}} s10 'struct (unnamed struct at {{.*}}cbuffer.hlsl:186:3
+  // CHECK: VarDecl {{.*}} s10 'struct (unnamed struct at {{.*}}cbuffer.hlsl:187:3
   // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_anon definition
   // CHECK: FieldDecl {{.*}} e 'float'
   // CHECK: CXXRecordDecl {{.*}} implicit referenced class __layout_anon_1 definition