fixup: Address Helena's feedback (2025-11-13)

Author: bogner

clang/lib/CodeGen/CGHLSLRuntime.cpp

@@ -267,9 +267,9 @@ CGHLSLRuntime::convertHLSLSpecificType(const Type *T,
   assert(T->isHLSLSpecificType() && "Not an HLSL specific type!");
 
   // Check if the target has a specific translation for this type first.
-  if (llvm::Type *LayoutTy =
+  if (llvm::Type *TargetTy =
           CGM.getTargetCodeGenInfo().getHLSLType(CGM, T, OffsetInfo))
-    return LayoutTy;
+    return TargetTy;
 
   llvm_unreachable("Generic handling of HLSL types is not supported.");
 }
@@ -326,9 +326,10 @@ void CGHLSLRuntime::emitBufferGlobalsAndMetadata(
     DeclsWithOffset.emplace_back(VD, OffsetInfo[OffsetIdx++]);
   }
 
-  llvm::stable_sort(DeclsWithOffset, [](const auto &LHS, const auto &RHS) {
-    return CGHLSLOffsetInfo::compareOffsets(LHS.second, RHS.second);
-  });
+  if (!OffsetInfo.empty())
+    llvm::stable_sort(DeclsWithOffset, [](const auto &LHS, const auto &RHS) {
+      return CGHLSLOffsetInfo::compareOffsets(LHS.second, RHS.second);
+    });
 
   // Associate the buffer global variable with its constants
   SmallVector<llvm::Metadata *> BufGlobals;
@@ -1148,7 +1149,7 @@ std::optional<LValue> CGHLSLRuntime::emitBufferArraySubscriptExpr(
   // be past the end of the in-memory object.
   SmallVector<llvm::Value *, 2> Indices;
   Indices.push_back(Idx);
-  Indices.push_back(llvm::ConstantInt::get(Indices[0]->getType(), 0));
+  Indices.push_back(llvm::ConstantInt::get(Idx->getType(), 0));
 
   llvm::Value *GEP = CGF.Builder.CreateGEP(LayoutTy, Addr.emitRawPointer(CGF),
                                            Indices, "cbufferidx");
@@ -1319,8 +1320,9 @@ LValue CGHLSLRuntime::emitBufferMemberExpr(CodeGenFunction &CGF,
   // Work out the buffer layout type to index into.
   QualType RecType = CGM.getContext().getCanonicalTagType(Rec);
   assert(RecType->isStructureOrClassType() && "Invalid type in HLSL buffer");
-  // Since this is a member of the buffer and not the buffer itself, we
-  // shouldn't have any offsets we need to contend with.
+  // Since this is a member of an object in the buffer and not the buffer's
+  // struct/class itself, we shouldn't have any offsets on the members we need
+  // to contend with.
   CGHLSLOffsetInfo EmptyOffsets;
   llvm::StructType *LayoutTy = HLSLBufferLayoutBuilder(CGM).layOutStruct(
       RecType->getAsCanonical<RecordType>(), EmptyOffsets);

clang/lib/CodeGen/HLSLBufferLayoutBuilder.cpp

@@ -52,9 +52,10 @@ HLSLBufferLayoutBuilder::layOutStruct(const RecordType *RT,
     for (const auto *FD : RD->fields())
       FieldsWithOffset.emplace_back(FD, OffsetInfo[OffsetIdx++]);
 
-  llvm::stable_sort(FieldsWithOffset, [](const auto &LHS, const auto &RHS) {
-    return CGHLSLOffsetInfo::compareOffsets(LHS.second, RHS.second);
-  });
+  if (!OffsetInfo.empty())
+    llvm::stable_sort(FieldsWithOffset, [](const auto &LHS, const auto &RHS) {
+      return CGHLSLOffsetInfo::compareOffsets(LHS.second, RHS.second);
+    });
 
   SmallVector<llvm::Type *> Layout;
   CharUnits CurrentOffset = CharUnits::Zero();
@@ -72,8 +73,6 @@ HLSLBufferLayoutBuilder::layOutStruct(const RecordType *RT,
 
     CharUnits NextOffset = CurrentOffset.alignTo(Align);
 
-    // TODO: Do we need to diagnose invalid packoffsets here, or will they have
-    // already been taken care of?
     if (Offset != CGHLSLOffsetInfo::Unspecified) {
       CharUnits PackOffset = CharUnits::fromQuantity(Offset);
       assert(PackOffset >= NextOffset &&

clang/lib/CodeGen/HLSLBufferLayoutBuilder.h

@@ -28,21 +28,17 @@ class HLSLBufferLayoutBuilder {
 public:
   HLSLBufferLayoutBuilder(CodeGenModule &CGM) : CGM(CGM) {}
 
-  /// Lays out a struct type following HLSL buffer rules and considering
-  /// PackOffsets, if provided. Previously created layout structs are cached by
-  /// CGHLSLRuntime.
+  /// Lays out a struct type following HLSL buffer rules and considering any
+  /// explicit offset information. Previously created layout structs are cached
+  /// by CGHLSLRuntime.
   ///
   /// The function iterates over all fields of the record type (including base
-  /// classes) and calls layoutField to converts each field to its corresponding
-  /// LLVM type and to calculate its HLSL constant buffer layout. Any embedded
-  /// structs (or arrays of structs) are converted to layout types as well.
+  /// classes) and works out a padded llvm type to represent the buffer layout.
   ///
-  /// When PackOffsets are specified the elements will be placed based on the
-  /// user-specified offsets. Not all elements must have a
-  /// packoffset/register(c#) annotation though. For those that don't, the
-  /// PackOffsets array will contain -1 value instead. These elements must be
-  /// placed at the end of the layout after all of the elements with specific
-  /// offset.
+  /// If a non-empty OffsetInfo is provided (ie, from `packoffset` annotations
+  /// in the source), any provided offsets offsets will be respected. If the
+  /// OffsetInfo is available but has empty entries, those will be layed out at
+  /// the end of the structure.
   llvm::StructType *layOutStruct(const RecordType *StructType,
                                  const CGHLSLOffsetInfo &OffsetInfo);
 

clang/test/CodeGenHLSL/GlobalConstructorFunction.hlsl

@@ -37,8 +37,7 @@ void main(unsigned GI : SV_GroupIndex) {}
 // INLINE-NEXT:   alloca
 // INLINE-NEXT:   store i32 12
 // INLINE-NEXT:   store i32 13
-// INLINE-NEXT:   %[[HANDLE:.*]] = call target("dx.CBuffer", %"__cblayout_$Globals")
-// INLINE-NEXT-SAME: @"llvm.dx.resource.handlefromimplicitbinding.tdx.CBuffer_s___cblayout_$Globalsst"(i32 0, i32 0, i32 1, i32 0, i1 false, ptr @"$Globals.str")
+// INLINE-NEXT:   %[[HANDLE:.*]] = call target("dx.CBuffer", %"__cblayout_$Globals") @"llvm.dx.resource.handlefromimplicitbinding.tdx.CBuffer_s___cblayout_$Globalsst"(i32 0, i32 0, i32 1, i32 0, ptr @"$Globals.str")
 // INLINE-NEXT:   store target("dx.CBuffer", %"__cblayout_$Globals") %[[HANDLE]], ptr @"$Globals.cb", align 4
 // INLINE-NEXT:   %0 = call i32 @llvm.dx.flattened.thread.id.in.group()
 // INLINE-NEXT:   store i32 %

clang/test/CodeGenHLSL/resources/cbuffer.hlsl

@@ -33,8 +33,10 @@
 // CHECK: %__cblayout_CBStructs = type <{
 // CHECK-SAME:   %A, target("dx.Padding", 8),
 
-// TODO: We should have target("dx.Padding", 2) padding after %B, but we don't correctly handle
-// 2- and 3-element vectors inside structs yet because of DataLayout rules.
+// TODO: We should have target("dx.Padding", 2) padding after %B, but we don't
+// correctly handle 2- and 3-element vectors inside structs yet because of
+// DataLayout rules. See https://github.com/llvm/llvm-project/issues/123968.
+//
 // CHECK-SAME: %B,
 
 // CHECK-SAME:   %C, target("dx.Padding", 8),