https://github.com/andykaylor created https://github.com/llvm/llvm-project/pull/200904
The `valuesToReload` handling in our `RunCleanupScope::forceCleanup()` function was not taking into account cleanup scopes for deferred conditional cleanups that get created when we call `forceDeactivation()`. This was leading to a CIR verification error in cases where a deferred cleanup was used in an expression that returns a value. This change adds code to spill values ahead of the `forceDeactivation()` call when we see that there are cleanups on the deferred stack. Assisted-by: Cursor / claude-opus-4.7 >From e1cfa6b06a229afaf149565711c72764734ac7a0 Mon Sep 17 00:00:00 2001 From: Andy Kaylor <[email protected]> Date: Fri, 29 May 2026 15:08:06 -0700 Subject: [PATCH] [CIR] Spill and reload values across deferred cleanup scopes The `valuesToReload` handling in our `RunCleanupScope::forceCleanup()` function was not taking into account cleanup scopes for deferred conditional cleanups that get created when we call `forceDeactivation()`. This was leading to a CIR verification error in cases where a deferred cleanup was used in an expression that returns a value. This change adds code to spill values ahead of the `forceDeactivation()` call when we see that there are cleanups on the deferred stack. Assisted-by: Cursor / claude-opus-4.7 --- clang/lib/CIR/CodeGen/CIRGenFunction.h | 61 ++++++++++++++++++- .../CodeGen/cleanup-derived-to-base-ref.cpp | 48 +++++++++++++++ 2 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 clang/test/CIR/CodeGen/cleanup-derived-to-base-ref.cpp diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h index 3d369dc7e7294..1e9be3dc2174e 100644 --- a/clang/lib/CIR/CodeGen/CIRGenFunction.h +++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h @@ -1198,9 +1198,66 @@ class CIRGenFunction : public CIRGenTypeCache { void forceCleanup(ArrayRef<mlir::Value *> valuesToReload = {}) { assert(performCleanup && "Already forced cleanup"); cgf.didCallStackSave = oldDidCallStackSave; + + // forceDeactivate() can pop cleanup scopes that were pushed with + // deferred deactivation, which moves the insertion point out of the + // cleanup body region. Any caller value defined inside such a body + // would no longer dominate uses past the scope. The downstream + // popCleanupBlocks() handles the spill for any cleanups it pops + // itself, but it cannot help with cleanups that forceDeactivate has + // already popped. Spill those values here, while the insertion point + // is still inside the body, so we can reload them after all popping + // is done. We only spill values whose defining op lives inside a + // cir.cleanup.scope, since values defined outside any cleanup scope + // (e.g. allocas in the entry block) already dominate the post-scope + // insertion point. + const bool hasPendingDeactivations = + cgf.deferredDeactivationCleanupStack.size() > + deactivateCleanups.oldDeactivateCleanupStackSize; + + llvm::SmallVector<Address> tempAllocas; + bool didSpillAny = false; + if (hasPendingDeactivations) { + tempAllocas.reserve(valuesToReload.size()); + for (mlir::Value *valPtr : valuesToReload) { + mlir::Value val = *valPtr; + if (!val || !val.getDefiningOp() || + !val.getDefiningOp()->getParentOfType<cir::CleanupScopeOp>()) { + tempAllocas.push_back(Address::invalid()); + continue; + } + Address temp = cgf.createDefaultAlignTempAlloca( + val.getType(), val.getLoc(), "tmp.exprcleanup"); + tempAllocas.push_back(temp); + cgf.builder.createStore(val.getLoc(), val, temp); + didSpillAny = true; + } + } + deactivateCleanups.forceDeactivate(); - cgf.popCleanupBlocks(cleanupStackDepth, lifetimeExtendedCleanupStackSize, - valuesToReload); + // If we already spilled some of the caller's values, don't ask + // popCleanupBlocks to spill them again. Values we did not pre-spill + // are not inside any cir.cleanup.scope, so they cannot be invalidated + // by either forceDeactivate's or popCleanupBlocks's pops (both only + // pop cir.cleanup.scope ops); they already dominate the post-scope + // insertion point on their own. + if (didSpillAny) { + cgf.popCleanupBlocks(cleanupStackDepth, + lifetimeExtendedCleanupStackSize); + + // Reload the spilled values now that all cleanup popping (and + // promotion of any lifetime-extended cleanups onto the EH stack) is + // done. + for (auto [addr, valPtr] : llvm::zip(tempAllocas, valuesToReload)) { + if (!addr.isValid()) + continue; + *valPtr = cgf.builder.createLoad(valPtr->getLoc(), addr); + } + } else { + cgf.popCleanupBlocks(cleanupStackDepth, + lifetimeExtendedCleanupStackSize, valuesToReload); + } + performCleanup = false; cgf.currentCleanupStackDepth = oldCleanupStackDepth; } diff --git a/clang/test/CIR/CodeGen/cleanup-derived-to-base-ref.cpp b/clang/test/CIR/CodeGen/cleanup-derived-to-base-ref.cpp new file mode 100644 index 0000000000000..8197f849a3d2e --- /dev/null +++ b/clang/test/CIR/CodeGen/cleanup-derived-to-base-ref.cpp @@ -0,0 +1,48 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -fcxx-exceptions -fexceptions -emit-cir %s -o %t.cir +// RUN: FileCheck --input-file=%t.cir %s --check-prefix=CIR +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -fcxx-exceptions -fexceptions -emit-llvm %s -o %t-cir.ll +// RUN: FileCheck --input-file=%t-cir.ll %s --check-prefix=LLVM + +struct Base {}; +struct Derived : Base { + ~Derived(); +}; + +Derived make_derived(); + +void bind_base_ref_to_derived_temp() { + const Base &r = make_derived(); +} + +// CIR-LABEL: cir.func {{.*}}@_Z29bind_base_ref_to_derived_tempv +// CIR: %[[REF_TMP:.*]] = cir.alloca !rec_Derived, !cir.ptr<!rec_Derived>, ["ref.tmp0"] +// CIR: %[[R:.*]] = cir.alloca !cir.ptr<!rec_Base>, !cir.ptr<!cir.ptr<!rec_Base>>, ["r", init, const] +// CIR: %[[SPILL:.*]] = cir.alloca !cir.ptr<!rec_Base>, !cir.ptr<!cir.ptr<!rec_Base>>, ["tmp.exprcleanup"] +// CIR: cir.call @_Z12make_derivedv() +// CIR: cir.cleanup.scope { +// CIR: %[[BASE:.*]] = cir.base_class_addr %[[REF_TMP]] : !cir.ptr<!rec_Derived> nonnull [0] -> !cir.ptr<!rec_Base> +// CIR: cir.store {{.*}} %[[BASE]], %[[SPILL]] +// CIR: cir.yield +// CIR: } cleanup eh { +// CIR: cir.call @_ZN7DerivedD1Ev(%[[REF_TMP]]) +// CIR: cir.yield +// CIR: } +// CIR: cir.cleanup.scope { +// CIR: %[[RELOAD:.*]] = cir.load {{.*}} %[[SPILL]] : !cir.ptr<!cir.ptr<!rec_Base>>, !cir.ptr<!rec_Base> +// CIR: cir.store {{.*}} %[[RELOAD]], %[[R]] +// CIR: cir.yield +// CIR: } cleanup all { +// CIR: cir.call @_ZN7DerivedD1Ev(%[[REF_TMP]]) +// CIR: cir.yield +// CIR: } +// CIR: cir.return + +// LLVM-LABEL: define {{.*}}@_Z29bind_base_ref_to_derived_tempv() +// LLVM: %[[REF_TMP:.*]] = alloca %struct.Derived +// LLVM: %[[R:.*]] = alloca ptr +// LLVM: %[[SPILL:.*]] = alloca ptr +// LLVM: store ptr %[[REF_TMP]], ptr %[[SPILL]] +// LLVM: %[[RELOAD:.*]] = load ptr, ptr %[[SPILL]] +// LLVM: store ptr %[[RELOAD]], ptr %[[R]] +// LLVM: call void @_ZN7DerivedD1Ev(ptr {{.*}} %[[REF_TMP]]) +// LLVM: ret void _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
