[CIR] Implement NRVO variable cleanup #170774

andykaylor · 2025-12-04T23:51:19Z

This implements the cleanup handling for C++ NRVO variables. Because exception handling is still incomplete, this doesn't behave any differently with exceptions enabled yet, but the NRVO-specific code for that case is trivial.

This implements the cleanup handling for C++ NRVO variables. Because exception handling is still incomplete, this doesn't behave any differently with exceptions enabled yet, but the code for that case is trivial.

llvmbot · 2025-12-04T23:51:51Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: Andy Kaylor (andykaylor)

Changes

This implements the cleanup handling for C++ NRVO variables. Because exception handling is still incomplete, this doesn't behave any differently with exceptions enabled yet, but the NRVO-specific code for that case is trivial.

Full diff: https://github.com/llvm/llvm-project/pull/170774.diff

4 Files Affected:

(modified) clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h (+8)
(modified) clang/include/clang/CIR/MissingFeatures.h (-1)
(modified) clang/lib/CIR/CodeGen/CIRGenDecl.cpp (+22-3)
(modified) clang/test/CIR/CodeGen/nrvo.cpp (+13)

diff --git a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
index c2598a1bc9f7b..aa47c4bce189b 100644
--- a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
+++ b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
@@ -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);
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index b8df0528ea18d..826a4b13f5c0c 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -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; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
index e93ddd9b8a32d..3431761abcd92 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);
           assert(builder.getInsertionBlock());
           builder.createStore(loc, falseNVRO, nrvoFlag);
 
@@ -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();
+    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;
@@ -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);
   }
 };
 
diff --git a/clang/test/CIR/CodeGen/nrvo.cpp b/clang/test/CIR/CodeGen/nrvo.cpp
index 05cd79ea1caf9..0fc9b9ba54012 100644
--- a/clang/test/CIR/CodeGen/nrvo.cpp
+++ b/clang/test/CIR/CodeGen/nrvo.cpp
@@ -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]]
 
@@ -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]]

andykaylor · 2025-12-04T23:54:44Z

clang/lib/CIR/CodeGen/CIRGenDecl.cpp

          cir::ConstantOp falseNVRO = builder.getFalse(loc);
          Address nrvoFlag = createTempAlloca(falseNVRO.getType(),
                                              CharUnits::One(), loc, "nrvo",
-                                              /*arraySize=*/nullptr, &address);


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.

AmrDeveloper

LGTM with nit

AmrDeveloper · 2025-12-05T19:21:07Z

clang/lib/CIR/CodeGen/CIRGenDecl.cpp

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


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

llvm-ci · 2025-12-06T00:28:08Z

LLVM Buildbot has detected a new failure on builder llvm-clang-aarch64-darwin running on doug-worker-4 while building clang at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/32291

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests/5/14' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/unittests/Support/./SupportTests-LLVM-Unit-23435-5-14.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=14 GTEST_SHARD_INDEX=5 /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/unittests/Support/./SupportTests
--

Script:
--
/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/unittests/Support/./SupportTests --gtest_filter=FileSystemTest.permissions
--
Test Directory: /tmp/lit-tmp-2hbnmzme/file-system-test-a5f4ca
/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2359: Failure
Value of: CheckPermissions(fs::set_gid_on_exe)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2374: Failure
Value of: CheckPermissions(fs::set_uid_on_exe | fs::set_gid_on_exe | fs::sticky_bit)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2381: Failure
Value of: CheckPermissions(fs::all_read | fs::set_uid_on_exe | fs::set_gid_on_exe | fs::sticky_bit)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2384: Failure
Value of: CheckPermissions(fs::all_perms)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2389: Failure
Value of: CheckPermissions(fs::all_perms & ~fs::sticky_bit)
  Actual: false
Expected: true


/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2359
Value of: CheckPermissions(fs::set_gid_on_exe)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2374
Value of: CheckPermissions(fs::set_uid_on_exe | fs::set_gid_on_exe | fs::sticky_bit)
  Actual: false
Expected: true

/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/unittests/Support/Path.cpp:2381
Value of: CheckPermissions(fs::all_read | fs::set_uid_on_exe | fs::set_gid_on_exe | fs::sticky_bit)
  Actual: false
...

This implements the cleanup handling for C++ NRVO variables. Because exception handling is still incomplete, this doesn't behave any differently with exceptions enabled yet, but the NRVO-specific code for that case is trivial.

[CIR] Implement NRVO variable cleanup

e1decb9

This implements the cleanup handling for C++ NRVO variables. Because exception handling is still incomplete, this doesn't behave any differently with exceptions enabled yet, but the code for that case is trivial.

andykaylor requested review from AmrDeveloper, bcardosolopes, erichkeane, lanza and xlauko December 4, 2025 23:51

llvmbot added clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project labels Dec 4, 2025

andykaylor commented Dec 4, 2025

View reviewed changes

AmrDeveloper approved these changes Dec 5, 2025

View reviewed changes

Address review feedback

ef57248

andykaylor enabled auto-merge (squash) December 5, 2025 23:32

andykaylor merged commit dedcf88 into llvm:main Dec 5, 2025
9 of 10 checks passed

andykaylor deleted the cir-nrvo-cleanup branch December 6, 2025 01:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CIR] Implement NRVO variable cleanup #170774

[CIR] Implement NRVO variable cleanup #170774

Uh oh!

andykaylor commented Dec 4, 2025

Uh oh!

llvmbot commented Dec 4, 2025 •

edited

Loading

Uh oh!

andykaylor Dec 4, 2025 •

edited

Loading

Uh oh!

AmrDeveloper left a comment

Uh oh!

AmrDeveloper Dec 5, 2025

Uh oh!

Uh oh!

llvm-ci commented Dec 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[CIR] Implement NRVO variable cleanup #170774

[CIR] Implement NRVO variable cleanup #170774

Uh oh!

Conversation

andykaylor commented Dec 4, 2025

Uh oh!

llvmbot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andykaylor Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper left a comment

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Dec 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

llvmbot commented Dec 4, 2025 •

edited

Loading

andykaylor Dec 4, 2025 •

edited

Loading