Address review feedback

andykaylor · andykaylor · commit 871d4628e634 · 2025-07-25T12:57:31.000-07:00
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -2260,7 +2260,9 @@ def CIR_ArrayCtor : CIR_ArrayInitDestroy<"array.ctor"> {
   let description = [{
     Initialize each array element using the same C++ constructor. This
     operation has one region, with one single block. The block has an
-    incoming argument for the current array index to initialize.
+    incoming argument for the current array element to initialize.
+
+    Example:
 
     ```mlir
     cir.array.ctor(%0 : !cir.ptr<!cir.array<!rec_S x 42>>) {
@@ -2277,7 +2279,9 @@ def CIR_ArrayDtor : CIR_ArrayInitDestroy<"array.dtor"> {
   let description = [{
     Destroy each array element using the same C++ destructor. This
     operation has one region, with one single block. The block has an
-    incoming argument for the current array index to initialize.
+    incoming argument for the current array element to destruct.
+
+    Example:
 
     ```mlir
     cir.array.dtor(%0 : !cir.ptr<!cir.array<!rec_S x 42>>) {
diff --git a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
@@ -659,28 +659,25 @@ void CIRGenFunction::emitNullabilityCheck(LValue lhs, mlir::Value rhs,
 void CIRGenFunction::emitArrayDestroy(mlir::Value begin, mlir::Value end,
                                       QualType elementType,
                                       CharUnits elementAlign,
-                                      Destroyer *destroyer,
-                                      bool checkZeroLength) {
+                                      Destroyer *destroyer) {
   assert(!elementType->isArrayType());
-  if (checkZeroLength)
-    cgm.errorNYI("emitArrayDestroy: check for zero length");
 
   // Differently from LLVM traditional codegen, use a higher level
   // representation instead of lowering directly to a loop.
   mlir::Type cirElementType = convertTypeForMem(elementType);
   cir::PointerType ptrToElmType = builder.getPointerTo(cirElementType);
 
   // Emit the dtor call that will execute for every array element.
-  builder.create<cir::ArrayDtor>(
-      *currSrcLoc, begin, [&](mlir::OpBuilder &b, mlir::Location loc) {
+  cir::ArrayDtor::create(
+      builder, *currSrcLoc, begin, [&](mlir::OpBuilder &b, mlir::Location loc) {
         auto arg = b.getInsertionBlock()->addArgument(ptrToElmType, loc);
         Address curAddr = Address(arg, cirElementType, elementAlign);
         assert(!cir::MissingFeatures::dtorCleanups());
 
         // Perform the actual destruction there.
         destroyer(*this, curAddr, elementType);
 
-        builder.create<cir::YieldOp>(loc);
+        cir::YieldOp::create(builder, loc);
       });
 }
 
@@ -702,25 +699,21 @@ void CIRGenFunction::emitDestroy(Address addr, QualType type,
   CharUnits elementAlign = addr.getAlignment().alignmentOfArrayElement(
       getContext().getTypeSizeInChars(type));
 
-  // Normally we have to check whether the array is zero-length.
-  bool checkZeroLength = true;
-
-  // But if the array length is constant, we can suppress that.
-  auto constantCount = dyn_cast<cir::ConstantOp>(length.getDefiningOp());
-  if (constantCount) {
-    auto constIntAttr = mlir::dyn_cast<cir::IntAttr>(constantCount.getValue());
-    // ...and if it's constant zero, we can just skip the entire thing.
-    if (constIntAttr && constIntAttr.getUInt() == 0)
-      return;
-    checkZeroLength = false;
-  } else {
+  auto constantCount = length.getDefiningOp<cir::ConstantOp>();
+  if (!constantCount) {
+    assert(!cir::MissingFeatures::vlas());
     cgm.errorNYI("emitDestroy: variable length array");
     return;
   }
 
+  auto constIntAttr = mlir::dyn_cast<cir::IntAttr>(constantCount.getValue());
+  // If it's constant zero, we can just skip the entire thing.
+  if (constIntAttr && constIntAttr.getUInt() == 0)
+    return;
+
   mlir::Value begin = addr.getPointer();
   mlir::Value end; // This will be used for future non-constant counts.
-  emitArrayDestroy(begin, end, type, elementAlign, destroyer, checkZeroLength);
+  emitArrayDestroy(begin, end, type, elementAlign, destroyer);
 
   // If the array destroy didn't use the length op, we can erase it.
   if (constantCount.use_empty())
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -850,7 +850,7 @@ class CIRGenFunction : public CIRGenTypeCache {
 
   void emitArrayDestroy(mlir::Value begin, mlir::Value end,
                         QualType elementType, CharUnits elementAlign,
-                        Destroyer *destroyer, bool checkZeroLength);
+                        Destroyer *destroyer);
 
   mlir::Value emitArrayLength(const clang::ArrayType *arrayType,
                               QualType &baseType, Address &addr);
diff --git a/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp b/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp
@@ -29,8 +29,8 @@ struct LoweringPreparePass : public LoweringPrepareBase<LoweringPreparePass> {
   void runOnOp(mlir::Operation *op);
   void lowerCastOp(cir::CastOp op);
   void lowerUnaryOp(cir::UnaryOp op);
-  void lowerArrayDtor(ArrayDtor op);
-  void lowerArrayCtor(ArrayCtor op);
+  void lowerArrayDtor(cir::ArrayDtor op);
+  void lowerArrayCtor(cir::ArrayCtor op);
 
   ///
   /// AST related
@@ -182,13 +182,14 @@ static void lowerArrayDtorCtorIntoLoop(cir::CIRBaseBuilderTy &builder,
   // PtrDiffTy and unify with CIRGen stuff.
   const unsigned sizeTypeSize =
       astCtx->getTypeSize(astCtx->getSignedSizeType());
-  mlir::Value numArrayElementsConst =
-      builder.getUnsignedInt(loc, arrayLen, sizeTypeSize);
-
-  auto begin = builder.create<cir::CastOp>(
-      loc, eltTy, cir::CastKind::array_to_ptrdecay, arrayAddr);
-  mlir::Value end = builder.create<cir::PtrStrideOp>(loc, eltTy, begin,
-                                                     numArrayElementsConst);
+  uint64_t endOffset = isCtor ? arrayLen : arrayLen - 1;
+  mlir::Value endOffsetVal =
+      builder.getUnsignedInt(loc, endOffset, sizeTypeSize);
+
+  auto begin = cir::CastOp::create(builder, loc, eltTy,
+                                   cir::CastKind::array_to_ptrdecay, arrayAddr);
+  mlir::Value end =
+      cir::PtrStrideOp::create(builder, loc, eltTy, begin, endOffsetVal);
   mlir::Value start = isCtor ? begin : end;
   mlir::Value stop = isCtor ? end : begin;
 
@@ -216,20 +217,16 @@ static void lowerArrayDtorCtorIntoLoop(cir::CIRBaseBuilderTy &builder,
         assert(ctorCall && "expected ctor call");
 
         // Array elements get constructed in order but destructed in reverse.
-        cir::PtrStrideOp nextElement;
-        if (isCtor) {
-          mlir::Value stride = builder.getUnsignedInt(loc, 1, sizeTypeSize);
-          ctorCall->moveBefore(stride.getDefiningOp());
-          ctorCall->setOperand(0, currentElement);
-          nextElement = builder.create<cir::PtrStrideOp>(
-              loc, eltTy, currentElement, stride);
-        } else {
-          mlir::Value stride = builder.getSignedInt(loc, -1, sizeTypeSize);
-          nextElement = builder.create<cir::PtrStrideOp>(
-              loc, eltTy, currentElement, stride);
-          ctorCall->moveAfter(nextElement);
-          ctorCall->setOperand(0, nextElement);
-        }
+        mlir::Value stride;
+        if (isCtor)
+          stride = builder.getUnsignedInt(loc, 1, sizeTypeSize);
+        else
+          stride = builder.getSignedInt(loc, -1, sizeTypeSize);
+
+        ctorCall->moveBefore(stride.getDefiningOp());
+        ctorCall->setOperand(0, currentElement);
+        auto nextElement = cir::PtrStrideOp::create(builder, loc, eltTy,
+                                                    currentElement, stride);
 
         // Store the element pointer to the temporary variable
         builder.createStore(loc, nextElement, tmpAddr);
@@ -240,11 +237,12 @@ static void lowerArrayDtorCtorIntoLoop(cir::CIRBaseBuilderTy &builder,
   op->erase();
 }
 
-void LoweringPreparePass::lowerArrayDtor(ArrayDtor op) {
+void LoweringPreparePass::lowerArrayDtor(cir::ArrayDtor op) {
   CIRBaseBuilderTy builder(getContext());
   builder.setInsertionPointAfter(op.getOperation());
 
   mlir::Type eltTy = op->getRegion(0).getArgument(0).getType();
+  assert(!cir::MissingFeatures::vlas());
   auto arrayLen =
       mlir::cast<cir::ArrayType>(op.getAddr().getType().getPointee()).getSize();
   lowerArrayDtorCtorIntoLoop(builder, astCtx, op, eltTy, op.getAddr(), arrayLen,
@@ -266,12 +264,12 @@ void LoweringPreparePass::lowerArrayCtor(cir::ArrayCtor op) {
 void LoweringPreparePass::runOnOp(mlir::Operation *op) {
   if (auto arrayCtor = dyn_cast<ArrayCtor>(op))
     lowerArrayCtor(arrayCtor);
+  else if (auto arrayDtor = dyn_cast<cir::ArrayDtor>(op))
+    lowerArrayDtor(arrayDtor);
   else if (auto cast = mlir::dyn_cast<cir::CastOp>(op))
     lowerCastOp(cast);
   else if (auto unary = mlir::dyn_cast<cir::UnaryOp>(op))
     lowerUnaryOp(unary);
-  else if (auto arrayDtor = dyn_cast<ArrayDtor>(op))
-    lowerArrayDtor(arrayDtor);
 }
 
 void LoweringPreparePass::runOnOperation() {
diff --git a/clang/test/CIR/CodeGen/array-dtor.cpp b/clang/test/CIR/CodeGen/array-dtor.cpp
@@ -25,16 +25,16 @@ void test_cleanup_array() {
 
 // CIR: cir.func{{.*}} @_Z18test_cleanup_arrayv()
 // CIR:   %[[S:.*]] = cir.alloca !cir.array<!rec_S x 42>, !cir.ptr<!cir.array<!rec_S x 42>>, ["s"]
-// CIR:   %[[CONST42:.*]] = cir.const #cir.int<42> : !u64i
+// CIR:   %[[CONST41:.*]] = cir.const #cir.int<41> : !u64i
 // CIR:   %[[DECAY:.*]] = cir.cast(array_to_ptrdecay, %[[S]] : !cir.ptr<!cir.array<!rec_S x 42>>), !cir.ptr<!rec_S>
-// CIR:   %[[END_PTR:.*]] = cir.ptr_stride(%[[DECAY]] : !cir.ptr<!rec_S>, %[[CONST42]] : !u64i), !cir.ptr<!rec_S>
+// CIR:   %[[END_PTR:.*]] = cir.ptr_stride(%[[DECAY]] : !cir.ptr<!rec_S>, %[[CONST41]] : !u64i), !cir.ptr<!rec_S>
 // CIR:   %[[ITER:.*]] = cir.alloca !cir.ptr<!rec_S>, !cir.ptr<!cir.ptr<!rec_S>>, ["__array_idx"]
 // CIR:   cir.store %[[END_PTR]], %[[ITER]] : !cir.ptr<!rec_S>, !cir.ptr<!cir.ptr<!rec_S>>
 // CIR:   cir.do {
 // CIR:     %[[CURRENT:.*]] = cir.load %[[ITER]] : !cir.ptr<!cir.ptr<!rec_S>>, !cir.ptr<!rec_S>
+// CIR:     cir.call @_ZN1SD1Ev(%[[CURRENT]]) nothrow : (!cir.ptr<!rec_S>) -> ()
 // CIR:     %[[CONST_MINUS1:.*]] = cir.const #cir.int<-1> : !s64i
 // CIR:     %[[NEXT:.*]] = cir.ptr_stride(%[[CURRENT]] : !cir.ptr<!rec_S>, %[[CONST_MINUS1]] : !s64i), !cir.ptr<!rec_S>
-// CIR:     cir.call @_ZN1SD1Ev(%[[NEXT]]) nothrow : (!cir.ptr<!rec_S>) -> ()
 // CIR:     cir.store %[[NEXT]], %[[ITER]] : !cir.ptr<!rec_S>, !cir.ptr<!cir.ptr<!rec_S>>
 // CIR:     cir.yield
 // CIR:   } while {
@@ -47,7 +47,7 @@ void test_cleanup_array() {
 // LLVM: define{{.*}} void @_Z18test_cleanup_arrayv()
 // LLVM:   %[[ARRAY:.*]] = alloca [42 x %struct.S]
 // LLVM:   %[[START:.*]] = getelementptr %struct.S, ptr %[[ARRAY]], i32 0
-// LLVM:   %[[END:.*]] = getelementptr %struct.S, ptr %[[START]], i64 42
+// LLVM:   %[[END:.*]] = getelementptr %struct.S, ptr %[[START]], i64 41
 // LLVM:   %[[ITER:.*]] = alloca ptr
 // LLVM:   store ptr %[[END]], ptr %[[ITER]]
 // LLVM:   br label %[[LOOP:.*]]
@@ -57,9 +57,9 @@ void test_cleanup_array() {
 // LLVM:   br i1 %[[DONE]], label %[[LOOP]], label %[[EXIT:.*]]
 // LLVM: [[LOOP]]:
 // LLVM:   %[[CURRENT:.*]] = load ptr, ptr %[[ITER]]
-// LLVM:   %[[LAST:.*]] = getelementptr %struct.S, ptr %[[CURRENT]], i64 -1
-// LLVM:   call void @_ZN1SD1Ev(ptr %[[LAST]])
-// LLVM:   store ptr %[[LAST]], ptr %[[ITER]]
+// LLVM:   call void @_ZN1SD1Ev(ptr %[[CURRENT]])
+// LLVM:   %[[NEXT:.*]] = getelementptr %struct.S, ptr %[[CURRENT]], i64 -1
+// LLVM:   store ptr %[[NEXT]], ptr %[[ITER]]
 // LLVM:   br label %[[COND]]
 // LLVM: [[EXIT]]:
 // LLVM:   ret void
