[CIR] Implement support for delete after new in a conditional branch#192544
Merged
Conversation
This implements handling for calling delete in an EH handler after a call to new when the new call appears inside a conditional operation, which requires the new result to be spilled inside the cleanup scope and reloaded after. This implementation introduces the DominatingValue helper class, which is adapted from classic codegen, but only the parts of that class that are needed for the current change are implemented. This will likely be expanded in a future change as other uses are added. Assisted-by: Cursor / claude-4.6-opus-high
andykaylor
requested review from
AmrDeveloper,
bcardosolopes,
erichkeane,
lanza and
xlauko
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) ChangesThis implements handling for calling delete in an EH handler after a call to new when the new call appears inside a conditional operation, which requires the new result to be spilled inside the cleanup scope and reloaded after. This implementation introduces the DominatingValue helper class, which is adapted from classic codegen, but only the parts of that class that are needed for the current change are implemented. This will likely be expanded in a future change as other uses are added. Assisted-by: Cursor / claude-4.6-opus-high Full diff: https://github.com/llvm/llvm-project/pull/192544.diff 5 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
index 55c27107e709d..816a39eb09060 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
@@ -86,6 +86,89 @@ Address CIRGenFunction::createCleanupActiveFlag() {
return active;
}
+void CIRGenFunction::initFullExprCleanup() {
+ initFullExprCleanupWithFlag(createCleanupActiveFlag());
+}
+
+void CIRGenFunction::initFullExprCleanupWithFlag(Address activeFlag) {
+ EHCleanupScope &cleanup = cast<EHCleanupScope>(*ehStack.begin());
+ assert(!cleanup.hasActiveFlag() && "cleanup already has active flag?");
+ cleanup.setActiveFlag(activeFlag);
+
+ if (cleanup.isNormalCleanup())
+ cleanup.setTestFlagInNormalCleanup();
+ if (cleanup.isEHCleanup())
+ cleanup.setTestFlagInEHCleanup();
+}
+
+//===----------------------------------------------------------------------===//
+// DominatingCIRValue / DominatingValue<RValue>
+//===----------------------------------------------------------------------===//
+
+bool DominatingCIRValue::needsSaving(mlir::Value value) {
+ if (!value)
+ return false;
+
+ // If it's a block argument, we don't need to save.
+ mlir::Operation *definingOp = value.getDefiningOp();
+ if (!definingOp)
+ return false;
+
+ // If the value is defined in the function entry block, it already dominates
+ // everything, so we don't need to save it.
+ mlir::Block *currBlock = definingOp->getBlock();
+ if (auto fnOp = definingOp->getParentOfType<cir::FuncOp>())
+ if (&fnOp.getBody().front() == currBlock)
+ return false;
+
+ return true;
+}
+
+DominatingCIRValue::saved_type DominatingCIRValue::save(CIRGenFunction &cgf,
+ mlir::Value value) {
+ if (!needsSaving(value))
+ return saved_type(value, false);
+
+ // Otherwise, we need an alloca.
+ CharUnits align = CharUnits::fromQuantity(
+ cgf.cgm.getDataLayout().getABITypeAlign(value.getType()));
+ mlir::Location loc = value.getLoc();
+ Address alloca =
+ cgf.createTempAlloca(value.getType(), align, loc, "cond-cleanup.save");
+ cgf.getBuilder().createStore(loc, value, alloca);
+
+ return saved_type(alloca.emitRawPointer(), true);
+}
+
+mlir::Value DominatingCIRValue::restore(CIRGenFunction &cgf,
+ saved_type value) {
+ // If the value says it wasn't saved, trust that it's still dominating.
+ if (!value.getInt())
+ return value.getPointer();
+
+ // Otherwise, it should be an alloca instruction, as set up in save().
+ auto alloca = value.getPointer().getDefiningOp<cir::AllocaOp>();
+ return cgf.getBuilder().createAlignedLoad(
+ alloca.getLoc(), alloca.getAllocaType(), alloca,
+ llvm::MaybeAlign(alloca.getAlignment()));
+}
+
+DominatingValue<RValue>::saved_type
+DominatingValue<RValue>::saved_type::save(CIRGenFunction &cgf, RValue rv) {
+ if (rv.isScalar())
+ return saved_type(DominatingCIRValue::save(cgf, rv.getValue()));
+
+ if (rv.isAggregate())
+ cgf.cgm.errorNYI("DominatingValue<RValue>::save for aggregate type");
+ else
+ cgf.cgm.errorNYI("DominatingValue<RValue>::save for complex type");
+ return saved_type(DominatingCIRValue::saved_type());
+}
+
+RValue DominatingValue<RValue>::saved_type::restore(CIRGenFunction &cgf) {
+ return RValue::get(DominatingCIRValue::restore(cgf, val));
+}
+
CIRGenFunction::FullExprCleanupScope::FullExprCleanupScope(CIRGenFunction &cgf,
const Expr *subExpr)
: cgf(cgf), cleanups(cgf), scope(nullptr),
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp b/clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp
index 30a833564ec2f..e77c4294e1ba3 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp
@@ -798,12 +798,12 @@ class CallDeleteDuringNew final
PlacementArg<Traits> *getPlacementArgs() { return getTrailingObjects(); }
+public:
void setPlacementArg(unsigned i, RValueTy argValue, QualType argType) {
assert(i < numPlacementArgs && "index out of range");
getPlacementArgs()[i] = {argValue, argType};
}
-public:
static size_t getExtraSize(size_t numPlacementArgs) {
return TrailingObj::template additionalSizeToAlloc<PlacementArg<Traits>>(
numPlacementArgs);
@@ -813,18 +813,11 @@ class CallDeleteDuringNew final
const FunctionDecl *operatorDelete, ValueTy ptr,
ValueTy allocSize,
const ImplicitAllocationParameters &iap,
- CharUnits allocAlign, const CallArgList *newArgs,
- unsigned numNonPlacementArgs, CIRGenFunction *cgf,
- mlir::Location loc)
+ CharUnits allocAlign)
: numPlacementArgs(numPlacementArgs),
passAlignmentToPlacementDelete(isAlignedAllocation(iap.PassAlignment)),
operatorDelete(operatorDelete), ptr(ptr), allocSize(allocSize),
- allocAlign(allocAlign) {
- for (unsigned i = 0, n = numPlacementArgs; i != n; ++i) {
- const CallArg &arg = (*newArgs)[i + numNonPlacementArgs];
- setPlacementArg(i, arg.getRValue(*cgf, loc), arg.ty);
- }
- }
+ allocAlign(allocAlign) {}
void emit(CIRGenFunction &cgf, Flags flags) override {
const auto *fpt = operatorDelete->getType()->castAs<FunctionProtoType>();
@@ -901,17 +894,50 @@ static void enterNewDeleteCleanup(CIRGenFunction &cgf, const CXXNewExpr *e,
typedef CallDeleteDuringNew<DirectCleanupTraits> DirectCleanup;
assert(!cir::MissingFeatures::typeAwareAllocation());
- cgf.ehStack.pushCleanupWithExtra<DirectCleanup>(
+ DirectCleanup *cleanup = cgf.ehStack.pushCleanupWithExtra<DirectCleanup>(
EHCleanup, e->getNumPlacementArgs(), e->getOperatorDelete(),
newPtr.getPointer(), allocSize, e->implicitAllocationParameters(),
- allocAlign, &newArgs, numNonPlacementArgs, &cgf,
- cgf.getLoc(e->getSourceRange()));
+ allocAlign);
+ for (unsigned i = 0, n = e->getNumPlacementArgs(); i != n; ++i) {
+ const CallArg &arg = newArgs[i + numNonPlacementArgs];
+ cleanup->setPlacementArg(i, arg.getRValue(cgf, cgf.getLoc(e->getSourceRange())),
+ arg.ty);
+ }
return;
}
- cgf.cgm.errorNYI(e->getSourceRange(),
- "enterNewDeleteCleanup: conditional branch");
+ // Otherwise, we need to save all this stuff.
+ DominatingValue<RValue>::saved_type savedNewPtr =
+ DominatingValue<RValue>::save(cgf, RValue::get(newPtr.getPointer()));
+ DominatingValue<RValue>::saved_type savedAllocSize =
+ DominatingValue<RValue>::save(cgf, RValue::get(allocSize));
+
+ struct ConditionalCleanupTraits {
+ typedef DominatingValue<RValue>::saved_type ValueTy;
+ typedef DominatingValue<RValue>::saved_type RValueTy;
+ static RValue get(CIRGenFunction &cgf, ValueTy v) {
+ return v.restore(cgf);
+ }
+ };
+ typedef CallDeleteDuringNew<ConditionalCleanupTraits> ConditionalCleanup;
+
+ assert(!cir::MissingFeatures::typeAwareAllocation());
+ ConditionalCleanup *cleanup =
+ cgf.ehStack.pushCleanupWithExtra<ConditionalCleanup>(
+ EHCleanup, e->getNumPlacementArgs(), e->getOperatorDelete(),
+ savedNewPtr, savedAllocSize, e->implicitAllocationParameters(),
+ allocAlign);
+ for (unsigned i = 0, n = e->getNumPlacementArgs(); i != n; ++i) {
+ const CallArg &arg = newArgs[i + numNonPlacementArgs];
+ cleanup->setPlacementArg(
+ i,
+ DominatingValue<RValue>::save(
+ cgf, arg.getRValue(cgf, cgf.getLoc(e->getSourceRange()))),
+ arg.ty);
+ }
+
+ cgf.initFullExprCleanup();
}
static void storeAnyExprIntoOneUnit(CIRGenFunction &cgf, const Expr *init,
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index 7f91cba115f04..1905800554838 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -1094,6 +1094,11 @@ class CIRGenFunction : public CIRGenTypeCache {
/// true at the current insertion point (inside the conditional branch).
Address createCleanupActiveFlag();
+ /// Set up the last cleanup that was pushed as a conditional
+ /// full-expression cleanup.
+ void initFullExprCleanup();
+ void initFullExprCleanupWithFlag(Address activeFlag);
+
/// Promote a single pending cleanup entry onto the EH scope stack. If the
/// entry has a valid activeFlag, the cleanup is configured as conditional.
/// Defined in CIRGenDecl.cpp where the concrete cleanup types are visible.
@@ -2600,6 +2605,39 @@ class CIRGenFunction : public CIRGenTypeCache {
};
};
+/// Helper class with most of the code for saving a value for a
+/// conditional expression cleanup.
+struct DominatingCIRValue {
+ typedef llvm::PointerIntPair<mlir::Value, 1, bool> saved_type;
+
+ /// Answer whether the given value needs extra work to be saved.
+ static bool needsSaving(mlir::Value value);
+
+ static saved_type save(CIRGenFunction &cgf, mlir::Value value);
+ static mlir::Value restore(CIRGenFunction &cgf, saved_type value);
+};
+
+/// A specialization of DominatingValue for RValue.
+template <> struct DominatingValue<RValue> {
+ typedef RValue type;
+ class saved_type {
+ DominatingCIRValue::saved_type val;
+
+ saved_type(DominatingCIRValue::saved_type val) : val(val) {}
+
+ public:
+ static saved_type save(CIRGenFunction &cgf, RValue value);
+ RValue restore(CIRGenFunction &cgf);
+ };
+
+ static saved_type save(CIRGenFunction &cgf, type value) {
+ return saved_type::save(cgf, value);
+ }
+ static type restore(CIRGenFunction &cgf, saved_type value) {
+ return value.restore(cgf);
+ }
+};
+
} // namespace clang::CIRGen
#endif
diff --git a/clang/lib/CIR/CodeGen/EHScopeStack.h b/clang/lib/CIR/CodeGen/EHScopeStack.h
index 308d98f108101..9e7a917d1d6cf 100644
--- a/clang/lib/CIR/CodeGen/EHScopeStack.h
+++ b/clang/lib/CIR/CodeGen/EHScopeStack.h
@@ -25,6 +25,18 @@ namespace clang::CIRGen {
class CIRGenFunction;
+template <class T> struct InvariantValue {
+ using type = T;
+ using saved_type = T;
+ static bool needsSaving(type value) { return false; }
+ static saved_type save(CIRGenFunction &cgf, type value) { return value; }
+ static type restore(CIRGenFunction &cgf, saved_type value) { return value; }
+};
+
+/// A metaprogramming class for ensuring that a value will dominate an
+/// arbitrary position in a function.
+template <class T> struct DominatingValue : InvariantValue<T> {};
+
enum CleanupKind : unsigned {
/// Denotes a cleanup that should run when a scope is exited using exceptional
/// control flow (a throw statement leading to stack unwinding, ).
diff --git a/clang/test/CIR/CodeGen/new-delete.cpp b/clang/test/CIR/CodeGen/new-delete.cpp
index d0c8c7d851c70..71e3407001ddb 100644
--- a/clang/test/CIR/CodeGen/new-delete.cpp
+++ b/clang/test/CIR/CodeGen/new-delete.cpp
@@ -247,6 +247,171 @@ B *c() {
// OGCG: %[[EXN_INSERT_2:.*]] = insertvalue { ptr, i32 } %[[EXN_INSERT]], i32 %[[EHSELECTOR]], 1
// OGCG: resume { ptr, i32 } %[[EXN_INSERT_2]]
+struct C {
+ C();
+ ~C();
+};
+
+C *test_new_delete_conditional(bool cond) {
+ return cond ? new C : 0;
+}
+
+// CIR-LABEL: @_Z27test_new_delete_conditionalb
+// CIR: %[[CLEANUP_COND:.*]] = cir.alloca !cir.bool, !cir.ptr<!cir.bool>, ["cleanup.cond"]
+// CIR: %[[FALSE:.*]] = cir.const #false
+// CIR: cir.store %[[FALSE]], %[[CLEANUP_COND]]
+// CIR: %[[TERN_RESULT:.*]] = cir.ternary
+// CIR: %[[PTR_SAVE:.*]] = cir.alloca !cir.ptr<!void>, !cir.ptr<!cir.ptr<!void>>, ["cond-cleanup.save"]
+// CIR: %[[SIZE_SAVE:.*]] = cir.alloca !u64i, !cir.ptr<!u64i>, ["cond-cleanup.save"]
+// CIR: %[[NEW_RESULT:.*]] = cir.alloca !cir.ptr<!rec_C>, !cir.ptr<!cir.ptr<!rec_C>>, ["__new_result"]
+// CIR: %[[ALLOC_SIZE:.*]] = cir.const #cir.int<1> : !u64i
+// CIR: %[[NEW_PTR:.*]] = cir.call @_Znwm(%[[ALLOC_SIZE]])
+// CIR: cir.store {{.*}}%[[NEW_PTR]], %[[PTR_SAVE]]
+// CIR: cir.store {{.*}}%[[ALLOC_SIZE]], %[[SIZE_SAVE]]
+// CIR: cir.cleanup.scope {
+// CIR: %[[TRUE:.*]] = cir.const #true
+// CIR: cir.store %[[TRUE]], %[[CLEANUP_COND]]
+// CIR: %[[NEW_AS_C:.*]] = cir.cast bitcast %[[NEW_PTR]] : !cir.ptr<!void> -> !cir.ptr<!rec_C>
+// CIR: cir.store{{.*}} %[[NEW_AS_C]], %[[NEW_RESULT]]
+// CIR: cir.call @_ZN1CC1Ev(%[[NEW_AS_C]])
+// CIR: cir.yield
+// CIR: } cleanup eh {
+// CIR: %[[FLAG:.*]] = cir.load {{.*}} %[[CLEANUP_COND]]
+// CIR: cir.if %[[FLAG]] {
+// CIR: %[[RESTORED_PTR:.*]] = cir.load {{.*}} %[[PTR_SAVE]]
+// CIR: cir.call @_ZdlPv(%[[RESTORED_PTR]])
+// CIR: }
+// CIR: cir.yield
+// CIR: }
+// CIR: %[[TRUE_RESULT:.*]] = cir.load{{.*}} %[[NEW_RESULT]]
+// CIR: cir.yield %[[TRUE_RESULT]] : !cir.ptr<!rec_C>
+// CIR: }, false {
+// CIR: %[[FALSE_RESULT:.*]] = cir.const #cir.ptr<null> : !cir.ptr<!rec_C>
+// CIR: cir.yield %[[FALSE_RESULT]] : !cir.ptr<!rec_C>
+// CIR: }) : (!cir.bool) -> !cir.ptr<!rec_C>
+
+// LLVM-LABEL: @_Z27test_new_delete_conditionalb
+// LLVM: store i8 0, ptr %[[CLEANUP_FLAG:.*]], align 1
+// LLVM: br i1 {{.*}}, label %[[TRUE_BB:.*]], label %[[FALSE_BB:.*]]
+// LLVM: [[TRUE_BB]]:
+// LLVM: %[[NEWP:.*]] = call ptr @_Znwm(i64 1) #[[ATTR_BUILTIN_NEW]]
+// LLVM: store ptr %[[NEWP]], ptr %[[SAVE_PTR:.*]], align 8
+// LLVM: store i8 1, ptr %[[CLEANUP_FLAG]], align 1
+// LLVM: invoke void @_ZN1CC1Ev(ptr {{.*}}%[[NEWP]])
+// LLVM: to label %{{.*}} unwind label %[[LPAD:.*]]
+// LLVM: [[LPAD]]:
+// LLVM: landingpad { ptr, i32 }
+// LLVM-NEXT: cleanup
+// LLVM: %[[IS_ACTIVE:.*]] = trunc i8 %{{.*}} to i1
+// LLVM: br i1 %[[IS_ACTIVE]], label %[[DO_DEL:.*]], label %[[SKIP_DEL:.*]]
+// LLVM: [[DO_DEL]]:
+// LLVM: %[[LOAD_PTR:.*]] = load ptr, ptr %[[SAVE_PTR]], align 8
+// LLVM: call void @_ZdlPv(ptr %[[LOAD_PTR]]) #[[ATTR_BUILTIN_DEL]]
+// LLVM: resume
+
+// OGCG-LABEL: @_Z27test_new_delete_conditionalb
+// OGCG: store i1 false, ptr %[[OG_FLAG:.*]], align 1
+// OGCG: br i1 {{.*}}, label %[[OG_TRUE:.*]], label %[[OG_FALSE:.*]]
+// OGCG: [[OG_TRUE]]:
+// OGCG: %[[OG_NEWP:.*]] = call noalias nonnull ptr @_Znwm(i64 1) #[[OGCG_ATTR_BUILTIN_NEW]]
+// OGCG: store ptr %[[OG_NEWP]], ptr %[[OG_SAVE:.*]], align 8
+// OGCG: store i1 true, ptr %[[OG_FLAG]], align 1
+// OGCG: invoke void @_ZN1CC1Ev(ptr {{.*}}%[[OG_NEWP]])
+// OGCG: to label %{{.*}} unwind label %[[OG_LPAD:.*]]
+// OGCG: [[OG_LPAD]]:
+// OGCG: landingpad { ptr, i32 }
+// OGCG-NEXT: cleanup
+// OGCG: %[[OG_ACTIVE:.*]] = load i1, ptr %[[OG_FLAG]], align 1
+// OGCG: br i1 %[[OG_ACTIVE]], label %[[OG_DO_DEL:.*]], label %[[OG_SKIP_DEL:.*]]
+// OGCG: [[OG_DO_DEL]]:
+// OGCG: %[[OG_LOAD:.*]] = load ptr, ptr %[[OG_SAVE]], align 8
+// OGCG: call void @_ZdlPv(ptr %[[OG_LOAD]]) #[[OGCG_ATTR_BUILTIN_DEL]]
+// OGCG: resume
+
+void *operator new(unsigned long, int);
+void operator delete(void *, int);
+
+C *test_new_delete_conditional_with_placement(bool cond, int tag) {
+ return cond ? new (tag) C : 0;
+}
+
+// CIR-LABEL: @_Z42test_new_delete_conditional_with_placementbi
+// CIR: %[[CLEANUP_COND:.*]] = cir.alloca !cir.bool, !cir.ptr<!cir.bool>, ["cleanup.cond"]
+// CIR: %[[TERN_RESULT:.*]] = cir.ternary
+// CIR: %[[PTR_SAVE:.*]] = cir.alloca !cir.ptr<!void>, !cir.ptr<!cir.ptr<!void>>, ["cond-cleanup.save"]
+// CIR: %[[SIZE_SAVE:.*]] = cir.alloca !u64i, !cir.ptr<!u64i>, ["cond-cleanup.save"]
+// CIR: %[[TAG_SAVE:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["cond-cleanup.save"]
+// CIR: %[[NEW_RESULT:.*]] = cir.alloca !cir.ptr<!rec_C>, !cir.ptr<!cir.ptr<!rec_C>>, ["__new_result"]
+// CIR: %[[ONE:.*]] = cir.const #cir.int<1> : !u64i
+// CIR: %[[TAG_VAL:.*]] = cir.load{{.*}}
+// CIR: %[[NEW_PTR:.*]] = cir.call @_Znwmi(%[[ONE]], %[[TAG_VAL]])
+// CIR: cir.store{{.*}} %[[NEW_PTR]], %[[PTR_SAVE]]
+// CIR: cir.store{{.*}} %[[ONE]], %[[SIZE_SAVE]]
+// CIR: cir.cleanup.scope {
+// CIR: cir.store{{.*}} %[[TAG_VAL]], %[[TAG_SAVE]]
+// CIR: %[[TRUE:.*]] = cir.const #true
+// CIR: cir.store %[[TRUE]], %[[CLEANUP_COND]]
+// CIR: %[[NEW_AS_C:.*]] = cir.cast bitcast %[[NEW_PTR]] : !cir.ptr<!void> -> !cir.ptr<!rec_C>
+// CIR: cir.store{{.*}} %[[NEW_AS_C]], %[[NEW_RESULT]]
+// CIR: cir.call @_ZN1CC1Ev(%[[NEW_AS_C]])
+// CIR: } cleanup eh {
+// CIR: %[[FLAG:.*]] = cir.load{{.*}} %[[CLEANUP_COND]]
+// CIR: cir.if %[[FLAG]] {
+// CIR: %[[RESTORED_PTR:.*]] = cir.load {{.*}} %[[PTR_SAVE]]
+// CIR: %[[RESTORED_TAG:.*]] = cir.load {{.*}} %[[TAG_SAVE]]
+// CIR: cir.call @_ZdlPvi(%[[RESTORED_PTR]], %[[RESTORED_TAG]])
+// CIR: }
+// CIR: cir.yield
+// CIR: }
+// CIR: %[[TRUE_RESULT:.*]] = cir.load{{.*}} %[[NEW_RESULT]]
+// CIR: cir.yield %[[TRUE_RESULT]] : !cir.ptr<!rec_C>
+// CIR: }, false {
+// CIR: %[[FALSE_RESULT:.*]] = cir.const #cir.ptr<null> : !cir.ptr<!rec_C>
+// CIR: cir.yield %[[FALSE_RESULT]] : !cir.ptr<!rec_C>
+// CIR: }) : (!cir.bool) -> !cir.ptr<!rec_C>
+
+// LLVM-LABEL: @_Z42test_new_delete_conditional_with_placementbi
+// LLVM: store i8 0, ptr %[[PL_FLAG:.*]], align 1
+// LLVM: br i1 {{.*}}, label %[[PL_TRUE:.*]], label %[[PL_FALSE:.*]]
+// LLVM: [[PL_TRUE]]:
+// LLVM: %[[PL_TAG:.*]] = load i32, ptr %{{.*}}, align 4
+// LLVM: %[[PL_NEWP:.*]] = call ptr @_Znwmi(i64 1, i32 %[[PL_TAG]])
+// LLVM: store ptr %[[PL_NEWP]], ptr %[[PL_SAVE_PTR:.*]], align 8
+// LLVM: store i32 %[[PL_TAG]], ptr %[[PL_SAVE_TAG:.*]], align 4
+// LLVM: store i8 1, ptr %[[PL_FLAG]], align 1
+// LLVM: invoke void @_ZN1CC1Ev(ptr {{.*}}%[[PL_NEWP]])
+// LLVM: to label %{{.*}} unwind label %[[PL_LPAD:.*]]
+// LLVM: [[PL_LPAD]]:
+// LLVM: landingpad { ptr, i32 }
+// LLVM-NEXT: cleanup
+// LLVM: %[[PL_ACTIVE:.*]] = trunc i8 %{{.*}} to i1
+// LLVM: br i1 %[[PL_ACTIVE]], label %[[PL_DO_DEL:.*]], label %[[PL_SKIP_DEL:.*]]
+// LLVM: [[PL_DO_DEL]]:
+// LLVM: %[[PL_LOAD_PTR:.*]] = load ptr, ptr %[[PL_SAVE_PTR]], align 8
+// LLVM: %[[PL_LOAD_TAG:.*]] = load i32, ptr %[[PL_SAVE_TAG]], align 4
+// LLVM: invoke void @_ZdlPvi(ptr %[[PL_LOAD_PTR]], i32 %[[PL_LOAD_TAG]])
+
+// OGCG-LABEL: @_Z42test_new_delete_conditional_with_placementbi
+// OGCG: store i1 false, ptr %[[OGP_FLAG:.*]], align 1
+// OGCG: br i1 {{.*}}, label %[[OGP_TRUE:.*]], label %[[OGP_FALSE:.*]]
+// OGCG: [[OGP_TRUE]]:
+// OGCG: %[[OGP_TAG:.*]] = load i32, ptr %{{.*}}, align 4
+// OGCG: %[[OGP_NEWP:.*]] = call ptr @_Znwmi(i64 1, i32 %[[OGP_TAG]])
+// OGCG: store ptr %[[OGP_NEWP]], ptr %[[OGP_SAVE_PTR:.*]], align 8
+// OGCG: store i32 %[[OGP_TAG]], ptr %[[OGP_SAVE_TAG:.*]], align 4
+// OGCG: store i1 true, ptr %[[OGP_FLAG]], align 1
+// OGCG: invoke void @_ZN1CC1Ev(ptr {{.*}}%[[OGP_NEWP]])
+// OGCG: to label %{{.*}} unwind label %[[OGP_LPAD:.*]]
+// OGCG: [[OGP_LPAD]]:
+// OGCG: landingpad { ptr, i32 }
+// OGCG-NEXT: cleanup
+// OGCG: %[[OGP_ACTIVE:.*]] = load i1, ptr %[[OGP_FLAG]], align 1
+// OGCG: br i1 %[[OGP_ACTIVE]], label %[[OGP_DO_DEL:.*]], label %[[OGP_SKIP_DEL:.*]]
+// OGCG: [[OGP_DO_DEL]]:
+// OGCG: %[[OGP_LOAD_PTR:.*]] = load ptr, ptr %[[OGP_SAVE_PTR]], align 8
+// OGCG: %[[OGP_LOAD_TAG:.*]] = load i32, ptr %[[OGP_SAVE_TAG]], align 4
+// OGCG: invoke void @_ZdlPvi(ptr %[[OGP_LOAD_PTR]], i32 %[[OGP_LOAD_TAG]])
+
// LLVM-DAG: attributes #[[ATTR_BUILTIN_NEW]] = {{{.*}}builtin{{.*}}}
// LLVM-DAG: attributes #[[ATTR_BUILTIN_DEL]] = {{{.*}}builtin{{.*}}}
// OGCG-DAG: attributes #[[OGCG_ATTR_BUILTIN_NEW]] = {{{.*}}builtin{{.*}}}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
erichkeane
reviewed
Apr 16, 2026
|
|
||
| template <class T> struct InvariantValue { | ||
| using type = T; | ||
| using saved_type = T; |
Contributor
There was a problem hiding this comment.
silly question: why two aliases here?
Contributor
Author
There was a problem hiding this comment.
This class, as well as DominatingValue and DominatingCIRValue are basically copy and paste from classic codegen, and I have to admit that I only have a rough understanding of how they work. As I understand it this is a trivial fallback implementation that intentionally does nothing. It basically is establishing a pattern for what the derived classes do. As far as I can tell there is no real reason for type and saved_type to be different here other than that they will be different in some derived classes and having them different here (arguably) promotes readability.
I don't think this PR actually reaches any of these fallback implementations, but they will be needed later.
The basic idea is that we have some value at one point in the CIR and we need to reference it at some later point in the IR. The InvariantValue class is saying that it doesn't change and so nothing needs to be done to make it available at the later point. But for DominatingValue we may need to save it to a temporary location and reload it, which is what save and restore will do there.
The classic codegen implementation of pushFullExprCleanup uses the DominatingValue class to "save" all of the packed arguments, so it ends up getting used for things like QualType that fall back on the InvariantValue implementation. I'm not sure we're going to need that for CIR, so maybe I should strip in down a bit more for now?
Contributor
There was a problem hiding this comment.
I would suggest simplifying, we can add the complexity later if we need it.
Contributor
There was a problem hiding this comment.
Aggree with @erichkeane, also snake_case triggers me here
| using saved_type = T; | ||
| static bool needsSaving(type value) { return false; } | ||
| static saved_type save(CIRGenFunction &cgf, type value) { return value; } | ||
| static type restore(CIRGenFunction &cgf, saved_type value) { return value; } |
Contributor
There was a problem hiding this comment.
What do save and restore do differently?
erichkeane
approved these changes
Apr 17, 2026
Contributor
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Would love hte simplification of save/restore if it works well enough, either way LGTM.
xlauko
reviewed
Apr 17, 2026
| // If it's a block argument, we don't need to save. | ||
| mlir::Operation *definingOp = value.getDefiningOp(); | ||
| if (!definingOp) | ||
| return false; |
Contributor
There was a problem hiding this comment.
This is not correct no? a block arg of a non-entry block does not dominate an arbitrary later insertion point.
Contributor
Author
There was a problem hiding this comment.
You're right, of course. This is what the incubator did, and I hadn't questioned it.
Comment on lines
+117
to
+122
| // If the value is defined in the function entry block, it already dominates | ||
| // everything, so we don't need to save it. | ||
| mlir::Block *currBlock = definingOp->getBlock(); | ||
| if (auto fnOp = definingOp->getParentOfType<cir::FuncOp>()) | ||
| if (&fnOp.getBody().front() == currBlock) | ||
| return false; |
Contributor
There was a problem hiding this comment.
This on the other hand is too conservative and does not eliminate values like:
cir.func {
cir.scope {
%value // currBlock != fnOp.getBody().front()
}
// nothing more here
}
Contributor
Author
There was a problem hiding this comment.
I stripped out some things from the incubator implementation here because I didn't think I could hit them with the one case I'm implementing in this PR. I'm starting to think maybe this whole DominatingValue mechanism is more complex and potentially error prone than we need right now.
| allocAlign, &newArgs, numNonPlacementArgs, &cgf, | ||
| cgf.getLoc(e->getSourceRange())); | ||
| allocAlign); | ||
| for (unsigned i = 0, n = e->getNumPlacementArgs(); i != n; ++i) { |
Contributor
There was a problem hiding this comment.
llvm::seq does not apply here?
| EHCleanup, e->getNumPlacementArgs(), e->getOperatorDelete(), | ||
| savedNewPtr, savedAllocSize, e->implicitAllocationParameters(), | ||
| allocAlign); | ||
| for (unsigned i = 0, n = e->getNumPlacementArgs(); i != n; ++i) { |
|
|
||
| public: | ||
| static saved_type save(CIRGenFunction &cgf, RValue value); | ||
| RValue restore(CIRGenFunction &cgf); |
|
|
||
| template <class T> struct InvariantValue { | ||
| using type = T; | ||
| using saved_type = T; |
Contributor
There was a problem hiding this comment.
Aggree with @erichkeane, also snake_case triggers me here
andykaylor
commented
Apr 17, 2026
| cgf.cgm.errorNYI(e->getSourceRange(), | ||
| "enterNewDeleteCleanup: conditional branch"); | ||
| // Otherwise, we need to save all this stuff. | ||
| auto saveValue = [&](mlir::Value value) -> mlir::Value { |
Contributor
Author
There was a problem hiding this comment.
This is a much simpler implementation. It doesn't attempt to look for cases where the values don't need to be saved, but the optimizer should handle that. In particular, you'll see in the CIR checks for the test that we spill the size even when it's a constant, but that gets resolved during lowering to LLVM IR.
erichkeane
approved these changes
Apr 20, 2026
| cleanup.setActiveFlag(activeFlag); | ||
|
|
||
| if (cleanup.isNormalCleanup()) | ||
| cleanup.setTestFlagInNormalCleanup(); |
Contributor
There was a problem hiding this comment.
It might be cleaner/nicer to have these two functions take a value, so that this becomes:
cleanup.setTestFlagInNormalCleanup(cleanump.isNormalCleanup()? Though it IS a little weird to me that the ctor of EHCleanupScope can't do this? Should it?
Contributor
Author
There was a problem hiding this comment.
The logic for setting these flags is not very closely coupled with the creation of the cleanup. It would take a lot of restructuring to set it in the ctor. On the other hand, making the setTestFlag... functions take an argument is a simple change. I'll do that.
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) ClangClang.CodeGenHLSL/resources/resources-in-structs-array.hlslClang.CodeGenHLSL/resources/resources-in-structs-inheritance.hlslClang.CodeGenHLSL/resources/resources-in-structs.hlslIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
🪟 Windows x64 Test Results
Failed Tests(click on a test name to see its output) ClangClang.CodeGenHLSL/resources/resources-in-structs-array.hlslClang.CodeGenHLSL/resources/resources-in-structs-inheritance.hlslClang.CodeGenHLSL/resources/resources-in-structs.hlslIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
erichkeane
approved these changes
Apr 20, 2026
s-perron
pushed a commit
to s-perron/llvm-project
that referenced
this pull request
Apr 21, 2026
…lvm#192544) This implements handling for calling delete in an EH handler after a call to new when the new call appears inside a conditional operation, which requires the new result to be spilled inside the cleanup scope and reloaded after. This implementation introduces the DominatingValue helper class, which is adapted from classic codegen, but only the parts of that class that are needed for the current change are implemented. This will likely be expanded in a future change as other uses are added. Assisted-by: Cursor / claude-4.6-opus-high
KHicketts
pushed a commit
to KHicketts/llvm-project
that referenced
this pull request
Apr 30, 2026
…lvm#192544) This implements handling for calling delete in an EH handler after a call to new when the new call appears inside a conditional operation, which requires the new result to be spilled inside the cleanup scope and reloaded after. This implementation introduces the DominatingValue helper class, which is adapted from classic codegen, but only the parts of that class that are needed for the current change are implemented. This will likely be expanded in a future change as other uses are added. Assisted-by: Cursor / claude-4.6-opus-high
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implements handling for calling delete in an EH handler after a call to new when the new call appears inside a conditional operation, which requires the new result to be spilled inside the cleanup scope and reloaded after.
This implementation introduces the DominatingValue helper class, which is adapted from classic codegen, but only the parts of that class that are needed for the current change are implemented. This will likely be expanded in a future change as other uses are added.
Assisted-by: Cursor / claude-4.6-opus-high