Skip to content

[CIR] Implement NRVO variable cleanup #170774

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andykaylor merged 2 commits into from
Dec 5, 2025
Merged

[CIR] Implement NRVO variable cleanup #170774

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1decb9
[CIR] Implement NRVO variable cleanup
andykaylor Dec 4, 2025
ef57248
Address review feedback
andykaylor Dec 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,14 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
return cir::StoreOp::create(*this, loc, val, dst, isVolatile, align, order);
}

/// Emit a load from an boolean flag variable.
cir::LoadOp createFlagLoad(mlir::Location loc, mlir::Value addr) {
mlir::Type boolTy = getBoolTy();
if (boolTy != mlir::cast<cir::PointerType>(addr.getType()).getPointee())
addr = createPtrBitcast(addr, boolTy);
return createLoad(loc, addr, /*isVolatile=*/false, /*alignment=*/1);
}

cir::StoreOp createFlagStore(mlir::Location loc, bool val, mlir::Value dst) {
mlir::Value flag = getBool(val, loc);
return CIRBaseBuilderTy::createStore(loc, flag, dst);
Expand Down
1 change: 0 additions & 1 deletion clang/include/clang/CIR/MissingFeatures.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ 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; }
Expand Down
25 changes: 22 additions & 3 deletions clang/lib/CIR/CodeGen/CIRGenDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

@andykaylor andykaylor Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was wrong. It was overwriting the Address in this function that is tracking the variable we're emitting. The incubator passes a separate allocaAddr argument here, which is also wrong but that variable isn't used in the incubator so it doesn't cause problems. Classic codegen has an AllocaAddr variable, but it doesn't pass it to CreateTempAlloca in this part of the code.

/*arraySize=*/nullptr);
assert(builder.getInsertionBlock());
builder.createStore(loc, falseNVRO, nrvoFlag);

Expand Down Expand Up @@ -835,7 +835,24 @@ template <class Derived> struct DestroyNRVOVariable : EHScopeStack::Cleanup {
QualType ty;

void emit(CIRGenFunction &cgf, Flags flags) override {
assert(!cir::MissingFeatures::cleanupDestroyNRVOVariable());
// Along the exceptions path we always execute the dtor.
bool nrvo = flags.isForNormalCleanup() && nrvoFlag;

CIRGenBuilderTy &builder = cgf.getBuilder();
mlir::OpBuilder::InsertionGuard guard(builder);
mlir::Location loc = addr.getPointer().getLoc();
Copy link
Member

@AmrDeveloper AmrDeveloper Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If loc will be used only inside the if block, should we move it inside it 🤔

if (nrvo) {
// If we exited via NRVO, we skip the destructor call.
mlir::Value didNRVO = builder.createFlagLoad(loc, nrvoFlag);
mlir::Value notNRVO = builder.createNot(didNRVO);
cir::IfOp::create(builder, loc, notNRVO, /*withElseRegion=*/false,
[&](mlir::OpBuilder &b, mlir::Location) {
static_cast<Derived *>(this)->emitDestructorCall(cgf);
builder.createYield(loc);
});
} else {
static_cast<Derived *>(this)->emitDestructorCall(cgf);
}
}

virtual ~DestroyNRVOVariable() = default;
Expand All @@ -851,7 +868,9 @@ struct DestroyNRVOVariableCXX final
const CXXDestructorDecl *dtor;

void emitDestructorCall(CIRGenFunction &cgf) {
assert(!cir::MissingFeatures::cleanupDestroyNRVOVariable());
cgf.emitCXXDestructorCall(dtor, Dtor_Complete,
/*forVirtualBase=*/false,
/*delegating=*/false, addr, ty);
}
};

Expand Down
13 changes: 13 additions & 0 deletions clang/test/CIR/CodeGen/nrvo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ NonTrivial test_nrvo() {
// CIR: cir.call @_Z10maybeThrowv() : () -> ()
// CIR: %[[TRUE:.*]] = cir.const #true
// CIR: cir.store{{.*}} %[[TRUE]], %[[NRVO_FLAG]]
// CIR: %[[NRVO_FLAG_VAL:.*]] = cir.load{{.*}} %[[NRVO_FLAG]]
// CIR: %[[NOT_NRVO_VAL:.*]] = cir.unary(not, %[[NRVO_FLAG_VAL]])
// CIR: cir.if %[[NOT_NRVO_VAL]] {
// CIR: cir.call @_ZN10NonTrivialD1Ev(%[[RESULT]])
// CIR: }
// CIR: %[[RET:.*]] = cir.load %[[RESULT]]
// CIR: cir.return %[[RET]]

Expand All @@ -81,6 +86,14 @@ NonTrivial test_nrvo() {
// LLVM: store i8 0, ptr %[[NRVO_FLAG]]
// LLVM: call void @_Z10maybeThrowv()
// LLVM: store i8 1, ptr %[[NRVO_FLAG]]
// LLVM: %[[NRVO_VAL:.*]] = load i8, ptr %[[NRVO_FLAG]]
// LLVM: %[[NRVO_VAL_TRUNC:.*]] = trunc i8 %[[NRVO_VAL]] to i1
// LLVM: %[[NOT_NRVO_VAL:.*]] = xor i1 %[[NRVO_VAL_TRUNC]], true
// LLVM: br i1 %[[NOT_NRVO_VAL]], label %[[NRVO_UNUSED:.*]], label %[[NRVO_USED:.*]]
// LLVM: [[NRVO_UNUSED]]:
// LLVM: call void @_ZN10NonTrivialD1Ev(ptr %[[RESULT]])
// LLVM: br label %[[NRVO_USED]]
// LLVM: [[NRVO_USED]]:
// LLVM: %[[RET:.*]] = load %struct.NonTrivial, ptr %[[RESULT]]
// LLVM: ret %struct.NonTrivial %[[RET]]

Expand Down
Loading