Address review feedback

Author: mmha

clang/lib/CIR/CodeGen/CIRGenBuilder.cpp

@@ -8,6 +8,7 @@
 
 #include "CIRGenBuilder.h"
 #include "mlir/IR/BuiltinAttributes.h"
+#include "clang/CIR/MissingFeatures.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/TypeSwitch.h"
 
@@ -132,21 +133,20 @@ void CIRGenBuilderTy::computeGlobalViewIndicesFromFlatOffset(
   computeGlobalViewIndicesFromFlatOffset(offset, subType, layout, indices);
 }
 
-static mlir::Type getAttributeType(mlir::Attribute attr) {
-  return mlir::cast<mlir::TypedAttr>(attr).getType();
-}
-
 cir::RecordType clang::CIRGen::CIRGenBuilderTy::getCompleteRecordType(
-    mlir::ArrayAttr fields, bool packed, bool padded, llvm::StringRef name,
-    const clang::RecordDecl *ast) {
-  llvm::SmallVector<mlir::Type, 8> members;
+    mlir::ArrayAttr fields, bool packed, bool padded, llvm::StringRef name) {
+  assert(!cir::MissingFeatures::astRecordDeclAttr());
+  llvm::SmallVector<mlir::Type> members;
   members.reserve(fields.size());
-  llvm::transform(fields, std::back_inserter(members), getAttributeType);
+  llvm::transform(fields, std::back_inserter(members),
+                  [](mlir::Attribute attr) {
+                    return mlir::cast<mlir::TypedAttr>(attr).getType();
+                  });
 
   if (name.empty())
     return getAnonRecordTy(members, packed, padded);
 
-  return getCompleteRecordTy(members, name, packed, padded);
+  return getCompleteNamedRecordType(members, packed, padded, name);
 }
 
 mlir::Attribute clang::CIRGen::CIRGenBuilderTy::getConstRecordOrZeroAttr(
@@ -155,11 +155,7 @@ mlir::Attribute clang::CIRGen::CIRGenBuilderTy::getConstRecordOrZeroAttr(
 
   // Record type not specified: create anon record type from members.
   if (!recordTy) {
-    llvm::SmallVector<mlir::Type, 8> members;
-    members.reserve(arrayAttr.size());
-    llvm::transform(arrayAttr, std::back_inserter(members), getAttributeType);
-    recordTy = getType<cir::RecordType>(members, packed, padded,
-                                        cir::RecordType::Struct);
+    recordTy = getCompleteRecordType(arrayAttr, packed, padded);
   }
 
   // Return zero or anonymous constant record.

clang/lib/CIR/CodeGen/CIRGenBuilder.h

@@ -138,9 +138,9 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
   ///
   /// If a record already exists and is complete, but the client tries to fetch
   /// it with a different set of attributes, this method will crash.
-  cir::RecordType getCompleteRecordTy(llvm::ArrayRef<mlir::Type> members,
-                                      llvm::StringRef name, bool packed,
-                                      bool padded) {
+  cir::RecordType getCompleteNamedRecordType(llvm::ArrayRef<mlir::Type> members,
+                                             bool packed, bool padded,
+                                             llvm::StringRef name) {
     const auto nameAttr = getStringAttr(name);
     auto kind = cir::RecordType::RecordKind::Struct;
     assert(!cir::MissingFeatures::astRecordDeclAttr());
@@ -165,8 +165,7 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
   cir::RecordType getCompleteRecordType(mlir::ArrayAttr fields,
                                         bool packed = false,
                                         bool padded = false,
-                                        llvm::StringRef name = "",
-                                        const clang::RecordDecl *ast = nullptr);
+                                        llvm::StringRef name = "");
 
   /// Get an incomplete CIR struct type. If we have a complete record
   /// declaration, we may create an incomplete type and then add the

clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp

@@ -109,8 +109,6 @@ class ConstantAggregateBuilder : private ConstantAggregateBuilderUtils {
   /// non-packed LLVM struct will give the correct layout.
   bool naturalLayout = true;
 
-  std::optional<size_t> splitAt(CharUnits pos);
-
   static mlir::Attribute
   buildFrom(CIRGenModule &cgm, ArrayRef<mlir::TypedAttr> elems,
             ArrayRef<CharUnits> offsets, CharUnits startOffset, CharUnits size,
@@ -167,51 +165,8 @@ bool ConstantAggregateBuilder::add(mlir::TypedAttr typedAttr, CharUnits offset,
   }
 
   // Uncommon case: constant overlaps what we've already created.
-  std::optional<size_t> firstElemToReplace = splitAt(offset);
-  if (!firstElemToReplace)
-    return false;
-
-  CharUnits cSize = getSize(typedAttr);
-  std::optional<size_t> lastElemToReplace = splitAt(offset + cSize);
-  if (!lastElemToReplace)
-    return false;
-
-  assert((firstElemToReplace == lastElemToReplace || allowOverwrite) &&
-         "unexpectedly overwriting field");
-
-  replace(elems, *firstElemToReplace, *lastElemToReplace, {typedAttr});
-  replace(offsets, *firstElemToReplace, *lastElemToReplace, {offset});
-  size = std::max(size, offset + cSize);
-  naturalLayout = false;
-  return true;
-}
-
-/// Returns a position within Elems and Offsets such that all elements
-/// before the returned index end before Pos and all elements at or after
-/// the returned index begin at or after Pos. Splits elements as necessary
-/// to ensure this. Returns None if we find something we can't split.
-std::optional<size_t> ConstantAggregateBuilder::splitAt(CharUnits pos) {
-  if (pos >= size)
-    return offsets.size();
-
-  clang::CharUnits *firstAfterPos = llvm::upper_bound(offsets, pos);
-  if (firstAfterPos == offsets.begin())
-    return 0;
-
-  // If we already have an element starting at Pos, we're done.
-  size_t lastAtOrBeforePosIndex = firstAfterPos - offsets.begin() - 1;
-  if (offsets[lastAtOrBeforePosIndex] == pos)
-    return lastAtOrBeforePosIndex;
-
-  // We found an element starting before Pos. Check for overlap.
-  mlir::TypedAttr c =
-      mlir::cast<mlir::TypedAttr>(elems[lastAtOrBeforePosIndex]);
-  if (offsets[lastAtOrBeforePosIndex] + getSize(c) <= pos)
-    return lastAtOrBeforePosIndex + 1;
-
-  // Try to decompose it into smaller constants.
-  cgm.errorNYI("split into smaller constants");
-  return std::nullopt;
+  cgm.errorNYI("overlapping constants");
+  return false;
 }
 
 mlir::Attribute ConstantAggregateBuilder::buildFrom(

clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp

@@ -627,9 +627,9 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *rd, cir::RecordType *ty) {
       CIRRecordLowering baseLowering(*this, rd, /*Packed=*/lowering.packed);
       baseLowering.lower(/*NonVirtualBaseType=*/true);
       std::string baseIdentifier = getRecordTypeName(rd, ".base");
-      baseTy =
-          builder.getCompleteRecordTy(baseLowering.fieldTypes, baseIdentifier,
-                                      baseLowering.packed, baseLowering.padded);
+      baseTy = builder.getCompleteNamedRecordType(
+          baseLowering.fieldTypes, baseLowering.packed, baseLowering.padded,
+          baseIdentifier);
       // TODO(cir): add something like addRecordTypeName
 
       // BaseTy and Ty must agree on their packedness for getCIRFieldNo to work