[HLSL] Add implicit resource element type concepts to AST #116413

bob80905 · 2024-11-15T17:37:32Z

This PR is step one on the journey to implement resource element type validation via C++20 concepts. The PR sets up the infrastructure for injecting implicit concept decls / concept specialization expressions into the AST, which will then be evaluated after template arguments are instantiated. This is not meant to be a complete implementation of the desired validation for HLSL,
there are a couple of missing elements:

We need the __builtin_hlsl_is_typed_resource_element_compatible builtin to be implemented.
We need other constraints, like is_intangible
We need to put the first 2 points together, and construct a finalized constraint expression, which should differ between typed and raw buffers
This is just an initial PR that puts some of the core infrastructure in place.

This PR is an edit of #112600, so that new tests that were put into main don't fail
Fixes #75676

llvmbot · 2024-11-16T06:20:26Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-hlsl

Author: Joshua Batista (bob80905)

Changes

This PR is step one on the journey to implement resource element type validation via C++20 concepts. The PR sets up the infrastructure for injecting implicit concept decls / concept specialization expressions into the AST, which will then be evaluated after template arguments are instantiated. This is not meant to be a complete implementation of the desired validation for HLSL,
there are a couple of missing elements:

We need the __builtin_hlsl_is_typed_resource_element_compatible builtin to be implemented.
We need other constraints, like is_intangible
We need to put the first 2 points together, and construct a finalized constraint expression, which should differ between typed and raw buffers
This is just an initial PR that puts some of the core infrastructure in place.

This PR is an edit of #112600, so that new tests that were put into main don't fail

Patch is 25.49 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/116413.diff

10 Files Affected:

(modified) clang/lib/Sema/HLSLExternalSemaSource.cpp (+199-11)
(modified) clang/test/AST/HLSL/AppendStructuredBuffer-AST.hlsl (+2-2)
(modified) clang/test/AST/HLSL/ConsumeStructuredBuffer-AST.hlsl (+2-2)
(modified) clang/test/AST/HLSL/RWBuffer-AST.hlsl (+18-2)
(modified) clang/test/AST/HLSL/RWStructuredBuffer-AST.hlsl (+2-2)
(modified) clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl (+2-2)
(modified) clang/test/AST/HLSL/StructuredBuffer-AST.hlsl (+2-2)
(added) clang/test/AST/HLSL/is_typed_resource_element_compatible_concept.hlsl (+10)
(modified) clang/test/SemaHLSL/BuiltIns/RWBuffers.hlsl (+14-2)
(modified) clang/test/SemaHLSL/BuiltIns/StructuredBuffers.hlsl (+2-2)

diff --git a/clang/lib/Sema/HLSLExternalSemaSource.cpp b/clang/lib/Sema/HLSLExternalSemaSource.cpp
index cac15b974a276e..400d3334f6f0de 100644
--- a/clang/lib/Sema/HLSLExternalSemaSource.cpp
+++ b/clang/lib/Sema/HLSLExternalSemaSource.cpp
@@ -289,8 +289,9 @@ struct BuiltinTypeDeclBuilder {
   }
 
   TemplateParameterListBuilder addTemplateArgumentList(Sema &S);
-  BuiltinTypeDeclBuilder &addSimpleTemplateParams(Sema &S,
-                                                  ArrayRef<StringRef> Names);
+  BuiltinTypeDeclBuilder &
+  addSimpleTemplateParams(Sema &S, ArrayRef<StringRef> Names, ConceptDecl *CD);
+  BuiltinTypeDeclBuilder &addConceptSpecializationExpr(Sema &S);
 };
 
 struct TemplateParameterListBuilder {
@@ -312,8 +313,9 @@ struct TemplateParameterListBuilder {
         S.Context, Builder.Record->getDeclContext(), SourceLocation(),
         SourceLocation(), /* TemplateDepth */ 0, Position,
         &S.Context.Idents.get(Name, tok::TokenKind::identifier),
-        /* Typename */ false,
-        /* ParameterPack */ false);
+        /* Typename */ true,
+        /* ParameterPack */ false,
+        /* HasTypeConstraint*/ false);
     if (!DefaultValue.isNull())
       Decl->setDefaultArgument(
           S.Context, S.getTrivialTemplateArgumentLoc(DefaultValue, QualType(),
@@ -323,19 +325,114 @@ struct TemplateParameterListBuilder {
     return *this;
   }
 
-  BuiltinTypeDeclBuilder &finalizeTemplateArgs() {
+  /*
+The concept specialization expression (CSE) constructed in
+constructConceptSpecializationExpr is constructed so that it
+matches the CSE that is constructed when parsing the below C++ code:
+template<typename T>
+concept is_typed_resource_element_compatible = sizeof(T) <= 16;
+template<typename element_type> requires
+is_typed_resource_element_compatible<element_type>
+struct RWBuffer {
+    element_type Val;
+};
+int fn() {
+    RWBuffer<int> Buf;
+}
+When dumping the AST and filtering for "RWBuffer", the resulting AST
+structure is what we're trying to construct below, specifically the
+CSE portion.
+*/
+  ConceptSpecializationExpr *
+  constructConceptSpecializationExpr(Sema &S, ConceptDecl *CD) {
+    ASTContext &Context = S.getASTContext();
+    SourceLocation Loc = Builder.Record->getBeginLoc();
+    DeclarationNameInfo DNI(CD->getDeclName(), Loc);
+    NestedNameSpecifierLoc NNSLoc;
+    DeclContext *DC = Builder.Record->getDeclContext();
+    TemplateArgumentListInfo TALI(Loc, Loc);
+
+    // Assume that the concept decl has just one template parameter
+    // This parameter should have been added when CD was constructed
+    // in getTypedBufferConceptDecl
+    assert(CD->getTemplateParameters()->size() == 1 &&
+           "unexpected concept decl parameter count");
+    TemplateTypeParmDecl *ConceptTTPD = dyn_cast<TemplateTypeParmDecl>(
+        CD->getTemplateParameters()->getParam(0));
+
+    // this TemplateTypeParmDecl is the template for the resource, and is
+    // used to construct a template argumentthat will be used
+    // to construct the ImplicitConceptSpecializationDecl
+    TemplateTypeParmDecl *T = TemplateTypeParmDecl::Create(
+        Context,                          // AST context
+        Builder.Record->getDeclContext(), // DeclContext
+        SourceLocation(), SourceLocation(),
+        /*depth=*/0,                // Depth in the template parameter list
+        /*position=*/0,             // Position in the template parameter list
+        /*id=*/nullptr,             // Identifier for 'T'
+        /*Typename=*/true,          // Indicates this is a 'typename' or 'class'
+        /*ParameterPack=*/false,    // Not a parameter pack
+        /*HasTypeConstraint=*/false // Has no type constraint
+    );
+
+    T->setDeclContext(DC);
+
+    QualType ConceptTType = Context.getTypeDeclType(ConceptTTPD);
+
+    // this is the 2nd template argument node, on which
+    // the concept constraint is actually being applied: 'element_type'
+    TemplateArgument ConceptTA = TemplateArgument(ConceptTType);
+
+    QualType CSETType = Context.getTypeDeclType(T);
+
+    // this is the 1st template argument node, which represents
+    // the abstract type that a concept would refer to: 'T'
+    TemplateArgument CSETA = TemplateArgument(CSETType);
+
+    ImplicitConceptSpecializationDecl *ImplicitCSEDecl =
+        ImplicitConceptSpecializationDecl::Create(
+            Context, Builder.Record->getDeclContext(), Loc, {CSETA});
+
+    // Constraint satisfaction is used to construct the
+    // ConceptSpecailizationExpr, and represents the 2nd Template Argument,
+    // located at the bottom of the sample AST above.
+    const ConstraintSatisfaction CS(CD, {ConceptTA});
+    TemplateArgumentLoc TAL = S.getTrivialTemplateArgumentLoc(
+        ConceptTA, QualType(), SourceLocation());
+
+    TALI.addArgument(TAL);
+    const ASTTemplateArgumentListInfo *ATALI =
+        ASTTemplateArgumentListInfo::Create(Context, TALI);
+
+    // In the concept reference, ATALI is what adds the extra
+    // TemplateArgument node underneath CSE
+    ConceptReference *CR =
+        ConceptReference::Create(Context, NNSLoc, Loc, DNI, CD, CD, ATALI);
+
+    ConceptSpecializationExpr *CSE =
+        ConceptSpecializationExpr::Create(Context, CR, ImplicitCSEDecl, &CS);
+
+    return CSE;
+  }
+
+  BuiltinTypeDeclBuilder &finalizeTemplateArgs(ConceptDecl *CD = nullptr) {
     if (Params.empty())
       return Builder;
+    ConceptSpecializationExpr *CSE =
+        CD ? constructConceptSpecializationExpr(S, CD) : nullptr;
+
     auto *ParamList = TemplateParameterList::Create(S.Context, SourceLocation(),
                                                     SourceLocation(), Params,
-                                                    SourceLocation(), nullptr);
+                                                    SourceLocation(), CSE);
     Builder.Template = ClassTemplateDecl::Create(
         S.Context, Builder.Record->getDeclContext(), SourceLocation(),
         DeclarationName(Builder.Record->getIdentifier()), ParamList,
         Builder.Record);
+
     Builder.Record->setDescribedClassTemplate(Builder.Template);
     Builder.Template->setImplicit(true);
     Builder.Template->setLexicalDeclContext(Builder.Record->getDeclContext());
+
     // NOTE: setPreviousDecl before addDecl so new decl replace old decl when
     // make visible.
     Builder.Template->setPreviousDecl(Builder.PrevTemplate);
@@ -355,13 +452,12 @@ BuiltinTypeDeclBuilder::addTemplateArgumentList(Sema &S) {
   return TemplateParameterListBuilder(S, *this);
 }
 
-BuiltinTypeDeclBuilder &
-BuiltinTypeDeclBuilder::addSimpleTemplateParams(Sema &S,
-                                                ArrayRef<StringRef> Names) {
+BuiltinTypeDeclBuilder &BuiltinTypeDeclBuilder::addSimpleTemplateParams(
+    Sema &S, ArrayRef<StringRef> Names, ConceptDecl *CD = nullptr) {
   TemplateParameterListBuilder Builder = this->addTemplateArgumentList(S);
   for (StringRef Name : Names)
     Builder.addTypeParameter(Name);
-  return Builder.finalizeTemplateArgs();
+  return Builder.finalizeTemplateArgs(CD);
 }
 
 HLSLExternalSemaSource::~HLSLExternalSemaSource() {}
@@ -472,10 +568,102 @@ static BuiltinTypeDeclBuilder setupBufferType(CXXRecordDecl *Decl, Sema &S,
       .addDefaultHandleConstructor(S);
 }
 
+BinaryOperator *constructSizeOfLEQ16Expr(ASTContext &Context,
+                                         SourceLocation NameLoc,
+                                         TemplateTypeParmDecl *T) {
+  // Obtain the QualType for 'unsigned long'
+  QualType UnsignedLongType = Context.UnsignedLongTy;
+
+  // Create a QualType that points to this TemplateTypeParmDecl
+  QualType TType = Context.getTypeDeclType(T);
+
+  // Create a TypeSourceInfo for the template type parameter 'T'
+  TypeSourceInfo *TTypeSourceInfo =
+      Context.getTrivialTypeSourceInfo(TType, NameLoc);
+
+  UnaryExprOrTypeTraitExpr *sizeOfExpr = new (Context) UnaryExprOrTypeTraitExpr(
+      UETT_SizeOf, TTypeSourceInfo, UnsignedLongType, NameLoc, NameLoc);
+
+  // Create an IntegerLiteral for the value '16' with size type
+  QualType SizeType = Context.getSizeType();
+  llvm::APInt SizeValue = llvm::APInt(Context.getTypeSize(SizeType), 16);
+  IntegerLiteral *SizeLiteral =
+      new (Context) IntegerLiteral(Context, SizeValue, SizeType, NameLoc);
+
+  QualType BoolTy = Context.BoolTy;
+
+  BinaryOperator *binaryOperator =
+      BinaryOperator::Create(Context, sizeOfExpr, // Left-hand side expression
+                             SizeLiteral,         // Right-hand side expression
+                             BO_LE,               // Binary operator kind (<=)
+                             BoolTy,              // Result type (bool)
+                             VK_LValue,           // Value kind
+                             OK_Ordinary,         // Object kind
+                             NameLoc,             // Source location of operator
+                             FPOptionsOverride());
+
+  return binaryOperator;
+}
+
+Expr *constructTypedBufferConstraintExpr(Sema &S, SourceLocation NameLoc,
+                                         TemplateTypeParmDecl *T) {
+  ASTContext &Context = S.getASTContext();
+
+  // first get the "sizeof(T) <= 16" expression, as a binary operator
+  BinaryOperator *SizeOfLEQ16 = constructSizeOfLEQ16Expr(Context, NameLoc, T);
+  // TODO: add the 'builtin_hlsl_is_typed_resource_element_compatible' builtin
+  // and return a binary operator that evaluates the builtin on the given
+  // template type parameter 'T'.
+  // Defined in issue https://github.com/llvm/llvm-project/issues/113223
+  return SizeOfLEQ16;
+}
+
+ConceptDecl *constructTypedBufferConceptDecl(Sema &S, NamespaceDecl *NSD) {
+  ASTContext &Context = S.getASTContext();
+  DeclContext *DC = NSD->getDeclContext();
+  SourceLocation DeclLoc = SourceLocation();
+
+  IdentifierInfo &ElementTypeII = Context.Idents.get("element_type");
+  TemplateTypeParmDecl *T = TemplateTypeParmDecl::Create(
+      Context, NSD->getDeclContext(), DeclLoc, DeclLoc,
+      /*depth=*/0,
+      /*position=*/0,
+      /*id=*/&ElementTypeII,
+      /*Typename=*/true,
+      /*ParameterPack=*/false);
+
+  T->setDeclContext(DC);
+  T->setReferenced();
+
+  // Create and Attach Template Parameter List to ConceptDecl
+  TemplateParameterList *ConceptParams = TemplateParameterList::Create(
+      Context, DeclLoc, DeclLoc, {T}, DeclLoc, nullptr);
+
+  DeclarationName DeclName = DeclarationName(
+      &Context.Idents.get("__is_typed_resource_element_compatible"));
+  Expr *ConstraintExpr = constructTypedBufferConstraintExpr(S, DeclLoc, T);
+
+  // Create a ConceptDecl
+  ConceptDecl *CD =
+      ConceptDecl::Create(Context, NSD->getDeclContext(), DeclLoc, DeclName,
+                          ConceptParams, ConstraintExpr);
+
+  // Attach the template parameter list to the ConceptDecl
+  CD->setTemplateParameters(ConceptParams);
+
+  // Add the concept declaration to the Translation Unit Decl
+  NSD->getDeclContext()->addDecl(CD);
+
+  return CD;
+}
+
 void HLSLExternalSemaSource::defineHLSLTypesWithForwardDeclarations() {
   CXXRecordDecl *Decl;
+  ConceptDecl *TypedBufferConcept =
+      constructTypedBufferConceptDecl(*SemaPtr, HLSLNamespace);
   Decl = BuiltinTypeDeclBuilder(*SemaPtr, HLSLNamespace, "RWBuffer")
-             .addSimpleTemplateParams(*SemaPtr, {"element_type"})
+             .addSimpleTemplateParams(*SemaPtr, {"element_type"},
+                                      TypedBufferConcept)
              .Record;
 
   onCompletion(Decl, [this](CXXRecordDecl *Decl) {
diff --git a/clang/test/AST/HLSL/AppendStructuredBuffer-AST.hlsl b/clang/test/AST/HLSL/AppendStructuredBuffer-AST.hlsl
index 5a13ca7735f999..8dbab44024d34d 100644
--- a/clang/test/AST/HLSL/AppendStructuredBuffer-AST.hlsl
+++ b/clang/test/AST/HLSL/AppendStructuredBuffer-AST.hlsl
@@ -12,7 +12,7 @@
 // instantiated specialization.
 
 // EMPTY: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit AppendStructuredBuffer
-// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> typename depth 0 index 0 element_type
 // EMPTY-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> class AppendStructuredBuffer
 // EMPTY-NEXT: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
 
@@ -26,7 +26,7 @@ AppendStructuredBuffer<int> Buffer;
 #endif
 
 // CHECK: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit AppendStructuredBuffer
-// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> typename depth 0 index 0 element_type
 // CHECK-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit class AppendStructuredBuffer definition
 
 // CHECK: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
diff --git a/clang/test/AST/HLSL/ConsumeStructuredBuffer-AST.hlsl b/clang/test/AST/HLSL/ConsumeStructuredBuffer-AST.hlsl
index b75f3fcb959cfc..f27941b539b6a8 100644
--- a/clang/test/AST/HLSL/ConsumeStructuredBuffer-AST.hlsl
+++ b/clang/test/AST/HLSL/ConsumeStructuredBuffer-AST.hlsl
@@ -12,7 +12,7 @@
 // instantiated specialization.
 
 // EMPTY: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit ConsumeStructuredBuffer
-// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> typename depth 0 index 0 element_type
 // EMPTY-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> class ConsumeStructuredBuffer
 // EMPTY-NEXT: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
 
@@ -26,7 +26,7 @@ ConsumeStructuredBuffer<int> Buffer;
 #endif
 
 // CHECK: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit ConsumeStructuredBuffer
-// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> typename depth 0 index 0 element_type
 // CHECK-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit class ConsumeStructuredBuffer definition
 
 // CHECK: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
diff --git a/clang/test/AST/HLSL/RWBuffer-AST.hlsl b/clang/test/AST/HLSL/RWBuffer-AST.hlsl
index ebddd72ddb1e0e..e3060a2dfa9677 100644
--- a/clang/test/AST/HLSL/RWBuffer-AST.hlsl
+++ b/clang/test/AST/HLSL/RWBuffer-AST.hlsl
@@ -11,7 +11,15 @@
 // instantiated specialization.
 
 // EMPTY: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit RWBuffer
-// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> typename depth 0 index 0 element_type
+// EMPTY-NEXT: ConceptSpecializationExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'bool' Concept 0x{{[0-9A-Fa-f]+}} '__is_typed_resource_element_compatible'
+// EMPTY-NEXT: ImplicitConceptSpecializationDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc>
+// EMPTY-NEXT: TemplateArgument type 'type-parameter-0-0'
+// EMPTY-NEXT: TemplateTypeParmType 0x{{[0-9A-Fa-f]+}} 'type-parameter-0-0' dependent depth 0 index 0
+// EMPTY-NEXT: TemplateTypeParm 0x{{[0-9A-Fa-f]+}} ''
+// EMPTY-NEXT: TemplateArgument type 'element_type':'type-parameter-0-0'
+// EMPTY-NEXT: TemplateTypeParmType 0x{{[0-9A-Fa-f]+}} 'element_type' dependent depth 0 index 0
+// EMPTY-NEXT: TemplateTypeParm 0x{{[0-9A-Fa-f]+}} 'element_type'
 // EMPTY-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> class RWBuffer
 // EMPTY-NEXT: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
 
@@ -25,7 +33,15 @@ RWBuffer<float> Buffer;
 #endif
 
 // CHECK: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit RWBuffer
-// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> typename depth 0 index 0 element_type
+// CHECK-NEXT: ConceptSpecializationExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'bool' Concept 0x{{[0-9A-Fa-f]+}} '__is_typed_resource_element_compatible'
+// CHECK-NEXT: ImplicitConceptSpecializationDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc>
+// CHECK-NEXT: TemplateArgument type 'type-parameter-0-0'
+// CHECK-NEXT: TemplateTypeParmType 0x{{[0-9A-Fa-f]+}} 'type-parameter-0-0' dependent depth 0 index 0
+// CHECK-NEXT: TemplateTypeParm 0x{{[0-9A-Fa-f]+}} ''
+// CHECK-NEXT: TemplateArgument type 'element_type':'type-parameter-0-0'
+// CHECK-NEXT: TemplateTypeParmType 0x{{[0-9A-Fa-f]+}} 'element_type' dependent depth 0 index 0
+// CHECK-NEXT: TemplateTypeParm 0x{{[0-9A-Fa-f]+}} 'element_type'
 // CHECK-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit class RWBuffer definition
 
 // CHECK: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
diff --git a/clang/test/AST/HLSL/RWStructuredBuffer-AST.hlsl b/clang/test/AST/HLSL/RWStructuredBuffer-AST.hlsl
index 4a1e1d7570e5e9..5058eab40b1aeb 100644
--- a/clang/test/AST/HLSL/RWStructuredBuffer-AST.hlsl
+++ b/clang/test/AST/HLSL/RWStructuredBuffer-AST.hlsl
@@ -12,7 +12,7 @@
 // instantiated specialization.
 
 // EMPTY: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit RWStructuredBuffer
-// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> typename depth 0 index 0 element_type
 // EMPTY-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> class RWStructuredBuffer
 // EMPTY-NEXT: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
 
@@ -26,7 +26,7 @@ RWStructuredBuffer<int> Buffer;
 #endif
 
 // CHECK: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit RWStructuredBuffer
-// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> typename depth 0 index 0 element_type
 // CHECK-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit class RWStructuredBuffer definition
 
 // CHECK: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
diff --git a/clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl b/clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl
index f334e1bb6db3fc..bd05432a09e01c 100644
--- a/clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl
+++ b/clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl
@@ -12,7 +12,7 @@
 // instantiated specialization.
 
 // EMPTY: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit RasterizerOrderedStructuredBuffer
-// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// EMPTY-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> typename depth 0 index 0 element_type
 // EMPTY-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> class RasterizerOrderedStructuredBuffer
 // EMPTY-NEXT: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
 
@@ -26,7 +26,7 @@ RasterizerOrderedStructuredBuffer<int> Buffer;
 #endif
 
 // CHECK: ClassTemplateDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit RasterizerOrderedStructuredBuffer
-// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> class depth 0 index 0 element_type
+// CHECK-NEXT: TemplateTypeParmDecl 0x{{[0-9A-Fa-f]+}} ...
[truncated]

damyanp · 2024-11-18T20:36:12Z

@bob80905 - can you associate this with an issue please?

llvm-beanz · 2024-11-18T22:28:34Z

clang/lib/Sema/HLSLExternalSemaSource.cpp

@@ -323,19 +325,114 @@ struct TemplateParameterListBuilder {
    return *this;
  }

-  BuiltinTypeDeclBuilder &finalizeTemplateArgs() {
+  /*
+The concept specialization expression (CSE) constructed in


nit: generally LLVM does per-line comment tokens for blocks (// rather than /* ... */)

…ject into add_concepts_to_ast_02

llvm-beanz · 2024-11-19T17:11:39Z

clang/lib/Sema/HLSLExternalSemaSource.cpp

+  // matches the CSE that is constructed when parsing the below C++ code:
+  //
+  // template<typename T>
+  // concept is_typed_resource_element_compatible = sizeof(T) <= 16;


Shouldn't this also require that T is a vector type?

I don't believe that's absolutely required. For example, one can have RWBuffer<float>, where the element type, float, is not a vector. If we require vectors, then I would think RWBuffer<float> would be invalid, and only things like RWBuffer<float2> would be accepted.

Additionally, this comment block will be changed when the finalized constraint expression is added, since the concept is more complex than just comparing the size to 16 bytes.

Sure, it needs to be a vector or a scalar (https://github.com/llvm/wg-hlsl/blob/main/proposals/0011-resource-element-type-validation.md?plain=1#L31).

I guess I maybe don't understand why we're breaking this into so many separate changes where the intermediate steps aren't fully testable.

The purpose of this change is to just verify concept validation works. A very simple constraint expression is used, size <= 16 bytes. This is tested by inspecting the AST, and seeing the CSE in the AST. It's also verified by running simple tests that fail when size > 16, and pass when <= 16.
I wanted to keep things separate because I want the PR to be kept as atomic as possible. This PR accomplishes the single purpose of introducing concept infrastructure and testing concept validation.
I intend the next and final PR to finalize the constraint expression, which will involve adding a bunch of tests that test each corner case of the constraint expression. This will also be a sizeable PR, which I figured would be best kept separate.

llvm-beanz

I think the way that you're staging these changes is awkward and results in more review time than we strictly need. We're also going to need to re-review a bunch of this code in a wider context in the near future, so I'm not sure I'm comfortable with this approach.

In a future change, you're going to introduce a new builtin (__builtin_hlsl_is_typed_resource_element_compatible), and a bunch of the code here will go away, and a bunch of the tests will be extended and updated to use that builtin. At that time we'll need to re-review this work.

Why not introduce the new builtin first so that this code can be merged in its completed state rather than in an intermediate state?

llvm-beanz · 2024-11-21T01:08:36Z

asa1qqqq

Looks like you added a file that has some odd characters and maybe a diff?

llvm-beanz · 2024-11-21T01:10:33Z

clang/lib/Sema/HLSLExternalSemaSource.cpp

+      Context, BoolTy, NameLoc, UTT_IsTypedResourceElementCompatible,
+      {TTypeSourceInfo}, NameLoc, true);
+
+  TypeTraitExpr *IsIntangibleExpr =


You don't need to check whether or not it is intangible, you only need to check if it is typed resource element compatible, because all intangible types are not typed resource element compatible.

…tangible

llvm-beanz

This looks great!

I know there was a lot of churn on this, but it came out really great. Thank you!

bogner · 2024-11-22T23:39:33Z

clang/lib/Sema/HLSLExternalSemaSource.cpp

-                                                  ArrayRef<StringRef> Names);
+  BuiltinTypeDeclBuilder &
+  addSimpleTemplateParams(Sema &S, ArrayRef<StringRef> Names, ConceptDecl *CD);
+  BuiltinTypeDeclBuilder &addConceptSpecializationExpr(Sema &S);


I think this is declared but never referenced.

bogner · 2024-11-22T23:41:44Z

clang/lib/Sema/HLSLExternalSemaSource.cpp

+        /*depth=*/0,                // Depth in the template parameter list
+        /*position=*/0,             // Position in the template parameter list
+        /*id=*/nullptr,             // Identifier for 'T'


I get that the parameter names in TemplateTypeParmDecl::Create aren't all great, but these comments really should match those names exactly.

bogner · 2024-11-22T23:43:03Z

clang/lib/Sema/HLSLExternalSemaSource.cpp

+  // Obtain the QualType for 'unsigned long'
+  QualType BoolTy = Context.BoolTy;


This comment is incorrect. It probably isn't really necessary anyway.

bogner · 2024-11-22T23:43:33Z

clang/lib/Sema/HLSLExternalSemaSource.cpp

+      /*depth=*/0,
+      /*position=*/0,
+      /*id=*/&ElementTypeII,


As before, please make sure these comments match the parameter names.

bogner · 2024-11-22T23:46:08Z

clang/lib/Sema/SemaExprCXX.cpp

-    if (Self.RequireCompleteType(TInfo->getTypeLoc().getBeginLoc(), T,
-                                 diag::err_incomplete_type))
+    if (T->isIncompleteType())


This doesn't look right - why did we need to change the general handling of evaluating type traits?

Consider RWBuffer<TemplatedBuffer<int> > r8;
If we left the old condition as it was, then in evaluating the outer RWBuffer, we'd need to require that the inner element type TemplatedBuffer<int> is a complete type. It is not yet, however. But we don't need to require that the element type is complete, because the type trait already rejects incomplete types.
Requiring that condition caused assertion errors IIRC when recursing into deeper element types that aren't yet complete. Types would attempt to be completed earlier than necessary. This is unnecessary since the type trait itself will return false on incomplete types.
"The only allowed types are builtin types or vectors (which are builtin), any incomplete type is guaranteed to not conform to the concept."

This PR adds concept validation to structured buffers, in the same way that it was done for typed buffers (like RWBuffer) in #116413. This PR should also be responsible for introducing rejection of 0 size elements for structured buffers. Fixes #117406

This PR adds concept validation to structured buffers, in the same way that it was done for typed buffers (like RWBuffer) in llvm/llvm-project#116413. This PR should also be responsible for introducing rejection of 0 size elements for structured buffers. Fixes llvm/llvm-project#117406

bob80905 added 3 commits November 15, 2024 09:00

update new tests

92ccbe7

fix test

f369fb5

Merge branch 'main' into add_concepts_to_ast_02

1a9b9fd

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels Nov 16, 2024

damyanp approved these changes Nov 18, 2024

View reviewed changes

llvm-beanz reviewed Nov 18, 2024

View reviewed changes

bob80905 added 2 commits November 18, 2024 14:33

use // instead of /*..*/ for comment block

8d173d3

Merge branch 'add_concepts_to_ast_02' of github.com:bob80905/llvm-pro…

554e3c6

…ject into add_concepts_to_ast_02

pow2clk mentioned this pull request Nov 18, 2024

[HLSL] Constrain matrix sizes using concepts #116710

Open

llvm-beanz reviewed Nov 19, 2024

View reviewed changes

add tests for rwbuffer concept validation

7c58efd

llvm-beanz reviewed Nov 21, 2024

View reviewed changes

asa1qqqq Outdated

Copy link

Collaborator

llvm-beanz Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you added a file that has some odd characters and maybe a diff?

llvm-beanz reviewed Nov 21, 2024

View reviewed changes

bob80905 added 4 commits November 20, 2024 18:08

remove extraneous file, simplify constraint expression by removing in…

4d15d89

…tangible

update tests

fa8d50e

remove require complete type, update tests

a8fae57

Merge branch 'main' into add_concepts_to_ast_02

66b0223

llvm-beanz approved these changes Nov 22, 2024

View reviewed changes

bob80905 merged commit ee0ca4e into llvm:main Nov 22, 2024
8 checks passed

bogner reviewed Nov 22, 2024

View reviewed changes

bob80905 mentioned this pull request Nov 23, 2024

[HLSL] Finalize Constraint expression for typed and raw buffer concepts. #113224

Closed

bob80905 mentioned this pull request Dec 12, 2024

[HLSL] Add concepts for Structured buffers #119643

Merged

		// Obtain the QualType for 'unsigned long'
		QualType BoolTy = Context.BoolTy;

[HLSL] Add implicit resource element type concepts to AST #116413

[HLSL] Add implicit resource element type concepts to AST #116413

Uh oh!

Conversation

bob80905 commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damyanp commented Nov 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bob80905 commented Nov 15, 2024 •

edited

Loading

llvmbot commented Nov 16, 2024 •

edited

Loading