Refactor: Address review comments for implicit typed buffer counters

This commit addresses review comments on the initial implementation of implicit typed buffer counters. The changes include:

- Refactored `CGHLSLBuiltins.cpp` to move `HandleTy` declaration into the SPIR-V specific block and add an early exit for the DXIL case.
- Updated `CGHLSLRuntime.cpp` to use a `const char *` for `CreateMethodName` to avoid string allocation.
- In `HLSLBuiltinTypeDeclBuilder.cpp`, replaced `if (Record->isCompleteDefinition())` with an assert and added descriptive comments to `addCreateFromBindingWithImplicitCounter` and `addCreateFromImplicitBindingWithImplicitCounter`.
- In `StructuredBuffers-constructors.hlsl`, updated the alignment check to be more specific and re-inserted a removed comment block.
- In `SemaHLSL.cpp`, changed `CreateMethodName` to `const char *`, removed a debug print, moved the re-creation of the binding object into the `else` block, and used `AST.getHLSLAttributedResourceType` for consistency.

Author: s-perron

clang/lib/CodeGen/CGHLSLBuiltins.cpp

@@ -353,17 +353,17 @@ Value *CodeGenFunction::EmitHLSLBuiltinExpr(unsigned BuiltinID,
     return Builder.CreateIntrinsic(HandleTy, IntrinsicID, Args);
   }
   case Builtin::BI__builtin_hlsl_resource_counterhandlefromimplicitbinding: {
-    llvm::Type *HandleTy = CGM.getTypes().ConvertType(E->getType());
     Value *MainHandle = EmitScalarExpr(E->getArg(0));
-    if (CGM.getTriple().isSPIRV()) {
-      Value *OrderID = EmitScalarExpr(E->getArg(1));
-      Value *SpaceOp = EmitScalarExpr(E->getArg(2));
-      llvm::Intrinsic::ID IntrinsicID =
-          llvm::Intrinsic::spv_resource_counterhandlefromimplicitbinding;
-      SmallVector<Value *> Args{MainHandle, OrderID, SpaceOp};
-      return Builder.CreateIntrinsic(HandleTy, IntrinsicID, Args);
-    }
-    return MainHandle;
+    if (!CGM.getTriple().isSPIRV())
+      return MainHandle;
+
+    llvm::Type *HandleTy = CGM.getTypes().ConvertType(E->getType());
+    Value *OrderID = EmitScalarExpr(E->getArg(1));
+    Value *SpaceOp = EmitScalarExpr(E->getArg(2));
+    llvm::Intrinsic::ID IntrinsicID =
+        llvm::Intrinsic::spv_resource_counterhandlefromimplicitbinding;
+    SmallVector<Value *> Args{MainHandle, OrderID, SpaceOp};
+    return Builder.CreateIntrinsic(HandleTy, IntrinsicID, Args);
   }
   case Builtin::BI__builtin_hlsl_resource_nonuniformindex: {
     Value *IndexOp = EmitScalarExpr(E->getArg(0));

clang/lib/CodeGen/CGHLSLRuntime.cpp

@@ -145,24 +145,19 @@ static CXXMethodDecl *lookupResourceInitMethodAndSetupArgs(
     // explicit binding
     auto *RegSlot = llvm::ConstantInt::get(CGM.IntTy, Binding.getSlot());
     Args.add(RValue::get(RegSlot), AST.UnsignedIntTy);
-    if (Binding.hasCounterImplicitOrderID())
-      CreateMethod = lookupMethod(
-          ResourceDecl, "__createFromBindingWithImplicitCounter", SC_Static);
-    else
-      CreateMethod =
-          lookupMethod(ResourceDecl, "__createFromBinding", SC_Static);
+    const char *Name = Binding.hasCounterImplicitOrderID()
+                           ? "__createFromBindingWithImplicitCounter"
+                           : "__createFromBinding";
+    CreateMethod = lookupMethod(ResourceDecl, Name, SC_Static);
   } else {
     // implicit binding
     auto *OrderID =
         llvm::ConstantInt::get(CGM.IntTy, Binding.getImplicitOrderID());
     Args.add(RValue::get(OrderID), AST.UnsignedIntTy);
-    if (Binding.hasCounterImplicitOrderID())
-      CreateMethod = lookupMethod(
-          ResourceDecl, "__createFromImplicitBindingWithImplicitCounter",
-          SC_Static);
-    else
-      CreateMethod =
-          lookupMethod(ResourceDecl, "__createFromImplicitBinding", SC_Static);
+    const char *Name = Binding.hasCounterImplicitOrderID()
+                           ? "__createFromImplicitBindingWithImplicitCounter"
+                           : "__createFromImplicitBinding";
+    CreateMethod = lookupMethod(ResourceDecl, Name, SC_Static);
   }
   Args.add(RValue::get(Space), AST.UnsignedIntTy);
   Args.add(RValue::get(Range), AST.IntTy);

clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.cpp

@@ -938,10 +938,24 @@ BuiltinTypeDeclBuilder &BuiltinTypeDeclBuilder::addCreateFromImplicitBinding() {
       .finalize();
 }
 
+// Adds static method that initializes resource from binding:
+//
+// static Resource<T>
+// __createFromBindingWithImplicitCounter(unsigned registerNo,
+//                                        unsigned spaceNo, int range,
+//                                        unsigned index, const char *name,
+//                                        unsigned counterOrderId) {
+//   Resource<T> tmp;
+//   tmp.__handle = __builtin_hlsl_resource_handlefrombinding(
+//       tmp.__handle, registerNo, spaceNo, range, index, name);
+//   tmp.__counter_handle =
+//       __builtin_hlsl_resource_counterhandlefromimplicitbinding(
+//           tmp.__handle, counterOrderId, spaceNo);
+//   return tmp;
+// }
 BuiltinTypeDeclBuilder &
 BuiltinTypeDeclBuilder::addCreateFromBindingWithImplicitCounter() {
-  if (Record->isCompleteDefinition())
-    return *this;
+  assert(!Record->isCompleteDefinition() && "record is already complete");
 
   using PH = BuiltinTypeMethodBuilder::PlaceHolder;
   ASTContext &AST = SemaRef.getASTContext();
@@ -971,10 +985,25 @@ BuiltinTypeDeclBuilder::addCreateFromBindingWithImplicitCounter() {
       .finalize();
 }
 
+// Adds static method that initializes resource from binding:
+//
+// static Resource<T>
+// __createFromImplicitBindingWithImplicitCounter(unsigned orderId,
+//                                                unsigned spaceNo, int range,
+//                                                unsigned index,
+//                                                const char *name,
+//                                                unsigned counterOrderId) {
+//   Resource<T> tmp;
+//   tmp.__handle = __builtin_hlsl_resource_handlefromimplicitbinding(
+//       tmp.__handle, orderId, spaceNo, range, index, name);
+//   tmp.__counter_handle =
+//       __builtin_hlsl_resource_counterhandlefromimplicitbinding(
+//           tmp.__handle, counterOrderId, spaceNo);
+//   return tmp;
+// }
 BuiltinTypeDeclBuilder &
 BuiltinTypeDeclBuilder::addCreateFromImplicitBindingWithImplicitCounter() {
-  if (Record->isCompleteDefinition())
-    return *this;
+  assert(!Record->isCompleteDefinition() && "record is already complete");
 
   using PH = BuiltinTypeMethodBuilder::PlaceHolder;
   ASTContext &AST = SemaRef.getASTContext();

clang/lib/Sema/SemaHLSL.cpp

@@ -3000,7 +3000,7 @@ bool SemaHLSL::CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
     auto MainAttrs = MainResType->getAttrs();
     assert(!MainAttrs.IsCounter && "cannot create a counter from a counter");
     MainAttrs.IsCounter = true;
-    QualType CounterHandleTy = getASTContext().getHLSLAttributedResourceType(
+    QualType CounterHandleTy = AST.getHLSLAttributedResourceType(
         MainResType->getWrappedType(), MainResType->getContainedType(),
         MainAttrs);
     TheCall->setType(CounterHandleTy);
@@ -3813,12 +3813,13 @@ void SemaHLSL::ActOnVariableDeclarator(VarDecl *VD) {
         uint32_t OrderID = getNextImplicitBindingOrderID();
         if (Binding.hasBinding())
           Binding.setImplicitOrderID(OrderID);
-        else
+        else {
           addImplicitBindingAttrToDecl(
               SemaRef, VD, getRegisterType(getResourceArrayHandleType(VD)),
               OrderID);
-        // Re-create the binding object to pick up the new attribute.
-        Binding = ResourceBindingAttrs(VD);
+          // Re-create the binding object to pick up the new attribute.
+          Binding = ResourceBindingAttrs(VD);
+        }
       }
 
       // Get to the base type of a potentially multi-dimensional array.
@@ -3855,7 +3856,7 @@ bool SemaHLSL::initGlobalResourceDecl(VarDecl *VD) {
   llvm::SmallVector<Expr *> Args;
 
   bool HasCounter = hasCounterHandle(ResourceDecl);
-  std::string CreateMethodName;
+  const char *CreateMethodName;
   if (Binding.isExplicit())
     CreateMethodName = HasCounter ? "__createFromBindingWithImplicitCounter"
                                   : "__createFromBinding";
@@ -3866,10 +3867,6 @@ bool SemaHLSL::initGlobalResourceDecl(VarDecl *VD) {
 
   CreateMethod =
       lookupMethod(SemaRef, ResourceDecl, CreateMethodName, VD->getLocation());
-  if (!CreateMethod) {
-    llvm::dbgs() << "STEVEN: failed to get method " << CreateMethodName << "\n";
-    VD->dumpColor();
-  }
 
   if (!CreateMethod)
     // This can happen if someone creates a struct that looks like an HLSL

clang/test/CodeGenHLSL/resources/StructuredBuffers-constructors.hlsl

@@ -38,7 +38,7 @@ export void foo() {
 // Buf1 initialization part 2 - body of StructuredBuffer<float>::::__createFromBinding
 
 // CHECK: define {{.*}} void @hlsl::StructuredBuffer<float>::__createFromBinding(unsigned int, unsigned int, int, unsigned int, char const*)
-// CHECK-SAME: ptr {{.*}} sret(%"class.hlsl::StructuredBuffer") align {{[0-9]+}} %[[RetValue1:.*]], i32 noundef %registerNo, 
+// CHECK-SAME: ptr {{.*}} sret(%"class.hlsl::StructuredBuffer") align {{(4|8)}} %[[RetValue1:.*]], i32 noundef %registerNo, 
 // CHECK-SAME: i32 noundef %spaceNo, i32 noundef %range, i32 noundef %index, ptr noundef %name)
 // CHECK: %[[Tmp1:.*]] = alloca %"class.hlsl::StructuredBuffer"
 // CHECK-DXIL: %[[Handle1:.*]] = call target("dx.RawBuffer", float, 0, 0) 
@@ -55,7 +55,7 @@ export void foo() {
 
 // Buf2 initialization part 2 - body of RWStructuredBuffer<float>::__createFromImplicitBindingWithImplicitCounter
 // CHECK: define linkonce_odr hidden void @hlsl::RWStructuredBuffer<float>::__createFromImplicitBindingWithImplicitCounter(unsigned int, unsigned int, int, unsigned int, char const*, unsigned int)
-// CHECK-SAME: (ptr {{.*}} sret(%"class.hlsl::RWStructuredBuffer") align {{[0-9]+}} %[[RetValue2:.*]], i32 noundef %orderId, 
+// CHECK-SAME: (ptr {{.*}} sret(%"class.hlsl::RWStructuredBuffer") align {{(4|8)}} %[[RetValue2:.*]], i32 noundef %orderId, 
 // CHECK-SAME: i32 noundef %spaceNo, i32 noundef %range, i32 noundef %index, ptr noundef %name, i32 noundef %counterOrderId)
 // CHECK: %[[Tmp2:.*]] = alloca %"class.hlsl::RWStructuredBuffer"
 // CHECK-DXIL: %[[Handle2:.*]] = call target("dx.RawBuffer", float, 1, 0)
@@ -77,6 +77,15 @@ export void foo() {
 // CHECK-SPV-NEXT: store target("spirv.VulkanBuffer", i32, 12, 1) %[[CounterHandle]], ptr %[[CounterHandlePtr]], align 8
 // CHECK: call void @hlsl::RWStructuredBuffer<float>::RWStructuredBuffer(hlsl::RWStructuredBuffer<float> const&)(ptr {{.*}} %[[RetValue2]], ptr {{.*}} %[[Tmp2]])
 
+// Buf3 initialization part 1 - local variable declared in function foo() is initialized by 
+// AppendStructuredBuffer<float> C1 default constructor
+// CHECK: define {{.*}}void @foo()
+// CHECK-NEXT: entry:
+// CHECK: %Buf3 = alloca %"class.hlsl::AppendStructuredBuffer", align {{4|8}}
+// CHECK-NEXT: call void @hlsl::AppendStructuredBuffer<float>::AppendStructuredBuffer()(ptr {{.*}} %Buf3)
+
+// Buf3 initialization part 2 - body of AppendStructuredBuffer<float> default C1 constructor that calls
+// the default C2 constructor
 // CHECK: define linkonce_odr hidden void @hlsl::StructuredBuffer<float>::StructuredBuffer()(ptr {{.*}} %this)
 // CHECK: call void @hlsl::StructuredBuffer<float>::StructuredBuffer()(ptr {{.*}} %this1)