From 92294ed052596f58589785573f45f5bf14e786a3 Mon Sep 17 00:00:00 2001 From: Andy Kaylor Date: Wed, 3 Dec 2025 14:11:19 -0800 Subject: [PATCH 1/3] [CIR] Upstream framework for NRVO cleanup This upstreams the framework code to handle NRVO cleanup. At this point the implementation follows the incubator in not performing the actual cleanup, but it populates the EH stack so that the cleanup can be done. This inserts a flag variable that tracks whether we are performing an NRVO return. Classic codegen checks this variable and calls a destructor if it is not set during normal cleanup. The check and destructor call are not yet implemented here. --- .../CIR/Dialect/Builder/CIRBaseBuilder.h | 5 ++ clang/include/clang/CIR/MissingFeatures.h | 1 + clang/lib/CIR/CodeGen/CIRGenDecl.cpp | 52 +++++++++++++++++-- clang/lib/CIR/CodeGen/CIRGenFunction.h | 4 ++ clang/lib/CIR/CodeGen/CIRGenStmt.cpp | 9 +++- clang/test/CIR/CodeGen/nrvo.cpp | 50 ++++++++++++++++++ 6 files changed, 117 insertions(+), 4 deletions(-) diff --git a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h index be9965ae3101f..c2598a1bc9f7b 100644 --- a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h +++ b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h @@ -331,6 +331,11 @@ class CIRBaseBuilderTy : public mlir::OpBuilder { return cir::StoreOp::create(*this, loc, val, dst, isVolatile, align, order); } + cir::StoreOp createFlagStore(mlir::Location loc, bool val, mlir::Value dst) { + mlir::Value flag = getBool(val, loc); + return CIRBaseBuilderTy::createStore(loc, flag, dst); + } + [[nodiscard]] cir::GlobalOp createGlobal(mlir::ModuleOp mlirModule, mlir::Location loc, mlir::StringRef name, diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h index 5075071661fb5..a8d35a6336ab9 100644 --- a/clang/include/clang/CIR/MissingFeatures.h +++ b/clang/include/clang/CIR/MissingFeatures.h @@ -227,6 +227,7 @@ struct MissingFeatures { static bool countedBySize() { return false; } static bool cgFPOptionsRAII() { return false; } static bool checkBitfieldClipping() { return false; } + static bool cleanupDestroyNRVOVariable() { return false; } static bool cirgenABIInfo() { return false; } static bool cleanupAfterErrorDiags() { return false; } static bool cleanupAppendInsts() { return false; } diff --git a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp index e0e4f67df87b2..304c42a1b5c67 100644 --- a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp @@ -98,8 +98,22 @@ CIRGenFunction::emitAutoVarAlloca(const VarDecl &d, if (const RecordDecl *rd = ty->getAsRecordDecl()) { if (const auto *cxxrd = dyn_cast(rd); (cxxrd && !cxxrd->hasTrivialDestructor()) || - rd->isNonTrivialToPrimitiveDestroy()) - cgm.errorNYI(d.getSourceRange(), "emitAutoVarAlloca: set NRVO flag"); + rd->isNonTrivialToPrimitiveDestroy()) { + // In LLVM: Create a flag that is used to indicate when the NRVO was + // applied to this variable. Set it to zero to indicate that NRVO was + // not applied. For now, use the same approach for CIRGen until we can + // be sure it's worth doing something more aggressive. + cir::ConstantOp falseNVRO = builder.getFalse(loc); + Address nrvoFlag = createTempAlloca(falseNVRO.getType(), + CharUnits::One(), loc, "nrvo", + /*ArraySize=*/nullptr, &address); + assert(builder.getInsertionBlock()); + builder.createStore(loc, falseNVRO, nrvoFlag); + + // Record the NRVO flag for this variable. + nrvoFlags[&d] = nrvoFlag.getPointer(); + emission.nrvoFlag = nrvoFlag.getPointer(); + } } } else { // A normal fixed sized variable becomes an alloca in the entry block, @@ -809,6 +823,35 @@ struct DestroyObject final : EHScopeStack::Cleanup { } }; +template struct DestroyNRVOVariable : EHScopeStack::Cleanup { + DestroyNRVOVariable(Address addr, QualType type, mlir::Value nrvoFlag) + : nrvoFlag(nrvoFlag), addr(addr), ty(type) {} + + mlir::Value nrvoFlag; + Address addr; + QualType ty; + + void emit(CIRGenFunction &cgf) override { + assert(!cir::MissingFeatures::cleanupDestroyNRVOVariable()); + } + + virtual ~DestroyNRVOVariable() = default; +}; + +struct DestroyNRVOVariableCXX final + : DestroyNRVOVariable { + DestroyNRVOVariableCXX(Address addr, QualType type, + const CXXDestructorDecl *dtor, mlir::Value nrvoFlag) + : DestroyNRVOVariable(addr, type, nrvoFlag), + dtor(dtor) {} + + const CXXDestructorDecl *dtor; + + void emitDestructorCall(CIRGenFunction &cgf) { + assert(!cir::MissingFeatures::cleanupDestroyNRVOVariable()); + } +}; + struct CallStackRestore final : EHScopeStack::Cleanup { Address stack; CallStackRestore(Address stack) : stack(stack) {} @@ -965,7 +1008,10 @@ void CIRGenFunction::emitAutoVarTypeCleanup( // If there's an NRVO flag on the emission, we need a different // cleanup. if (emission.nrvoFlag) { - cgm.errorNYI(var->getSourceRange(), "emitAutoVarTypeCleanup: NRVO"); + assert(!type->isArrayType()); + CXXDestructorDecl *dtor = type->getAsCXXRecordDecl()->getDestructor(); + ehStack.pushCleanup(cleanupKind, addr, type, dtor, + emission.nrvoFlag); return; } // Otherwise, this is handled below. diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h index 91b5ffa8b9ff9..daec4ec03e8b1 100644 --- a/clang/lib/CIR/CodeGen/CIRGenFunction.h +++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h @@ -123,6 +123,10 @@ class CIRGenFunction : public CIRGenTypeCache { GlobalDecl curSEHParent; + /// A mapping from NRVO variables to the flags used to indicate + /// when the NRVO has been applied to this variable. + llvm::DenseMap nrvoFlags; + llvm::DenseMap lambdaCaptureFields; clang::FieldDecl *lambdaThisCaptureField = nullptr; diff --git a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp index 7bb8c2153056a..9f4f681eb9a79 100644 --- a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp @@ -458,7 +458,14 @@ mlir::LogicalResult CIRGenFunction::emitReturnStmt(const ReturnStmt &s) { if (getContext().getLangOpts().ElideConstructors && s.getNRVOCandidate() && s.getNRVOCandidate()->isNRVOVariable()) { assert(!cir::MissingFeatures::openMP()); - assert(!cir::MissingFeatures::nrvo()); + // Apply the named return value optimization for this return statement, + // which means doing nothing: the appropriate result has already been + // constructed into the NRVO variable. + + // If there is an NRVO flag for this variable, set it to 1 into indicate + // that the cleanup code should not destroy the variable. + if (auto nrvoFlag = nrvoFlags[s.getNRVOCandidate()]) + builder.createFlagStore(loc, true, nrvoFlag); } else if (!rv) { // No return expression. Do nothing. } else if (rv->getType()->isVoidType()) { diff --git a/clang/test/CIR/CodeGen/nrvo.cpp b/clang/test/CIR/CodeGen/nrvo.cpp index ce08c795a77ee..fce0571a82107 100644 --- a/clang/test/CIR/CodeGen/nrvo.cpp +++ b/clang/test/CIR/CodeGen/nrvo.cpp @@ -49,3 +49,53 @@ struct S f1() { // OGCG-NEXT: call void @_ZN1SC1Ev(ptr {{.*}} %[[RETVAL]]) // OGCG-NEXT: %[[RET:.*]] = load i64, ptr %[[RETVAL]] // OGCG-NEXT: ret i64 %[[RET]] + +struct NonTrivial { + ~NonTrivial(); +}; + +void maybeThrow(); + +NonTrivial test_nrvo() { + NonTrivial result; + maybeThrow(); + return result; +} + +// TODO(cir): Handle normal cleanup properly. + +// CIR: cir.func {{.*}} @_Z9test_nrvov() +// CIR: %[[RESULT:.*]] = cir.alloca !rec_NonTrivial, !cir.ptr, ["__retval"] +// CIR: %[[NRVO_FLAG:.*]] = cir.alloca !cir.bool, !cir.ptr, ["nrvo"] +// CIR: %[[FALSE:.*]] = cir.const #false +// CIR: cir.store{{.*}} %[[FALSE]], %[[NRVO_FLAG]] +// CIR: cir.call @_Z10maybeThrowv() : () -> () +// CIR: %[[TRUE:.*]] = cir.const #true +// CIR: cir.store{{.*}} %[[TRUE]], %[[NRVO_FLAG]] +// CIR: %[[RET:.*]] = cir.load %[[RESULT]] +// CIR: cir.return %[[RET]] + +// LLVM: define {{.*}} %struct.NonTrivial @_Z9test_nrvov() +// LLVM: %[[RESULT:.*]] = alloca %struct.NonTrivial +// LLVM: %[[NRVO_FLAG:.*]] = alloca i8 +// LLVM: store i8 0, ptr %[[NRVO_FLAG]] +// LLVM: call void @_Z10maybeThrowv() +// LLVM: store i8 1, ptr %[[NRVO_FLAG]] +// LLVM: %[[RET:.*]] = load %struct.NonTrivial, ptr %[[RESULT]] +// LLVM: ret %struct.NonTrivial %[[RET]] + +// OGCG: define {{.*}} void @_Z9test_nrvov(ptr {{.*}} sret(%struct.NonTrivial) {{.*}} %[[RESULT:.*]]) +// OGCG: %[[RESULT_ADDR:.*]] = alloca ptr +// OGCG: %[[NRVO_FLAG:.*]] = alloca i1, align 1 +// OGCG: store ptr %[[RESULT]], ptr %[[RESULT_ADDR]] +// OGCG: store i1 false, ptr %[[NRVO_FLAG]] +// OGCG: call void @_Z10maybeThrowv() +// OGCG: store i1 true, ptr %[[NRVO_FLAG]] +// OGCG: %[[NRVO_VAL:.*]] = load i1, ptr %[[NRVO_FLAG]] +// OGCG: br i1 %[[NRVO_VAL]], label %[[SKIPDTOR:.*]], label %[[NRVO_UNUSED:.*]] +// OGCG: [[NRVO_UNUSED]]: +// OGCG: call void @_ZN10NonTrivialD1Ev(ptr {{.*}} %[[RESULT]]) +// OGCG: br label %[[SKIPDTOR]] +// OGCG: [[SKIPDTOR]]: +// OGCG: ret void + \ No newline at end of file From e5e793b09b73e4aea68213fd6a8cf6a07ffaa990 Mon Sep 17 00:00:00 2001 From: Andy Kaylor Date: Wed, 3 Dec 2025 15:14:28 -0800 Subject: [PATCH 2/3] Fix trailing space --- clang/test/CIR/CodeGen/nrvo.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/test/CIR/CodeGen/nrvo.cpp b/clang/test/CIR/CodeGen/nrvo.cpp index fce0571a82107..05cd79ea1caf9 100644 --- a/clang/test/CIR/CodeGen/nrvo.cpp +++ b/clang/test/CIR/CodeGen/nrvo.cpp @@ -98,4 +98,3 @@ NonTrivial test_nrvo() { // OGCG: br label %[[SKIPDTOR]] // OGCG: [[SKIPDTOR]]: // OGCG: ret void - \ No newline at end of file From 02054b2c243eea8c496a4899426e83880695995b Mon Sep 17 00:00:00 2001 From: Andy Kaylor Date: Thu, 4 Dec 2025 12:39:12 -0800 Subject: [PATCH 3/3] Address review feedback --- clang/lib/CIR/CodeGen/CIRGenDecl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp index 304c42a1b5c67..c9e10840778b4 100644 --- a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp @@ -106,7 +106,7 @@ CIRGenFunction::emitAutoVarAlloca(const VarDecl &d, cir::ConstantOp falseNVRO = builder.getFalse(loc); Address nrvoFlag = createTempAlloca(falseNVRO.getType(), CharUnits::One(), loc, "nrvo", - /*ArraySize=*/nullptr, &address); + /*arraySize=*/nullptr, &address); assert(builder.getInsertionBlock()); builder.createStore(loc, falseNVRO, nrvoFlag);