[ControlHeightReduction] Drop lifetime annotations where necessary #159686

boomanaiden154 · 2025-09-19T01:52:38Z

ControlHeightReduction will duplicate some blocks and insert phi nodes in exit blocks of regions that it operates on for any live values. This includes allocas. Having a lifetime annotation refer to a phi node was made illegal in 92c55a3, which causes the verifier to fail after CHR.

There are some cases where we might not need to drop lifetime annotations (usually because we do not need the phi to begin with), but drop all annotations for now to be conservative.

Fixes #159621.

ControlHeightReduction will duplicate some blocks and insert phi nodes in exit blocks of regions that it operates on for any live values. This includes allocas. Having a lifetime annotation refer to a phi node was made illegal in 92c55a3, which causes the verifier to fail after CHR. There are some cases where we might not need to drop lifetime annotations (usually because we do not need the phi to begin with), but drop all annotations for now to be conservative. I believe this also ends up being a correctness issue on top of the assertion because the lifetime annotation in some cases would get shifted to after the store, making the store UB when it was not before. Fixes llvm#159621.

llvmbot · 2025-09-19T01:53:15Z

@llvm/pr-subscribers-pgo

Author: Aiden Grossman (boomanaiden154)

Changes

ControlHeightReduction will duplicate some blocks and insert phi nodes in exit blocks of regions that it operates on for any live values. This includes allocas. Having a lifetime annotation refer to a phi node was made illegal in 92c55a3, which causes the verifier to fail after CHR.

There are some cases where we might not need to drop lifetime annotations (usually because we do not need the phi to begin with), but drop all annotations for now to be conservative. I believe this also ends up being a correctness issue on top of the assertion because the lifetime annotation in some cases would get shifted to after the store, making the store UB when it was not before.

Fixes #159621.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp (+6)
(modified) llvm/test/Transforms/PGOProfile/chr.ll (+59)

diff --git a/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp b/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
index c14bbecf0d4e1..3e05b8539dcb8 100644
--- a/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
+++ b/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
@@ -1592,6 +1592,12 @@ static void insertTrivialPHIs(CHRScope *Scope,
         TrivialPHIs.insert(PN);
         CHR_DEBUG(dbgs() << "Insert phi " << *PN << "\n");
         for (Instruction *UI : Users) {
+          // Drop lifetime annotations as it is illegal for them to refer to a
+          // phi node.
+          if (UI->isLifetimeStartOrEnd()) {
+            UI->eraseFromParent();
+            continue;
+          }
           for (unsigned J = 0, NumOps = UI->getNumOperands(); J < NumOps; ++J) {
             if (UI->getOperand(J) == &I) {
               UI->setOperand(J, PN);
diff --git a/llvm/test/Transforms/PGOProfile/chr.ll b/llvm/test/Transforms/PGOProfile/chr.ll
index f0a1574c5f209..860e40917a77f 100644
--- a/llvm/test/Transforms/PGOProfile/chr.ll
+++ b/llvm/test/Transforms/PGOProfile/chr.ll
@@ -2678,6 +2678,65 @@ bb6:
   ret void
 }
 
+; Test that when we create phi nodes for allocas, we drop lifetime annotations
+; that would otherwise be illegally referring to a phi node.
+declare void @baz(i64)
+declare void @llvm.lifetime.start.p0(ptr captures(none))
+define void @test_chr_with_lifetimes(ptr %i) !prof !14 {
+; CHECK-LABEL: @test_chr_with_lifetimes(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[I:%.*]], align 4
+; CHECK-NEXT:    [[DOTFR:%.*]] = freeze i32 [[TMP0]]
+; CHECK-NEXT:    [[DOTNOT:%.*]] = icmp eq i32 [[DOTFR]], 0
+; CHECK-NEXT:    br i1 [[DOTNOT]], label [[ENTRY_SPLIT_NONCHR:%.*]], label [[ENTRY_SPLIT:%.*]], !prof [[PROF21:![0-9]+]]
+; CHECK:       entry.split:
+; CHECK-NEXT:    [[TEST:%.*]] = alloca i32, align 8
+; CHECK-NEXT:    call void @baz(i64 0)
+; CHECK-NEXT:    call void @foo()
+; CHECK-NEXT:    br label [[BB1:%.*]]
+; CHECK:       entry.split.nonchr:
+; CHECK-NEXT:    [[TEST_NONCHR:%.*]] = alloca i32, align 8
+; CHECK-NEXT:    call void @baz(i64 4)
+; CHECK-NEXT:    br label [[BB1]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[TMP1:%.*]] = phi ptr [ [[TEST]], [[ENTRY_SPLIT]] ], [ [[TEST_NONCHR]], [[ENTRY_SPLIT_NONCHR]] ]
+; CHECK-NEXT:    store ptr [[TMP1]], ptr [[I]], align 8
+; CHECK-NEXT:    br label [[BB2:%.*]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[TMP2:%.*]] = phi ptr [ [[TMP3:%.*]], [[BB2]] ], [ null, [[BB1]] ]
+; CHECK-NEXT:    [[TMP3]] = getelementptr i8, ptr [[TMP2]], i64 24
+; CHECK-NEXT:    [[TMP4:%.*]] = icmp eq ptr [[TMP2]], [[I]]
+; CHECK-NEXT:    br i1 [[TMP4]], label [[BB3:%.*]], label [[BB2]]
+; CHECK:       bb3:
+; CHECK-NEXT:    ret void
+;
+entry:
+  %1 = load i32, ptr %i
+  %2 = icmp eq i32 %1, 0
+  %3 = select i1 %2, i64 4, i64 0, !prof !15
+  %test = alloca i32, align 8
+  call void @baz(i64 %3)
+  br i1 %2, label %bb1, label %bb0, !prof !15
+
+bb0:
+  call void @foo()
+  br label %bb1
+
+bb1:
+  call void @llvm.lifetime.start.p0(ptr %test)
+  store ptr %test, ptr %i, align 8
+  br label %bb2
+
+bb2:
+  %4 = phi ptr [ %5, %bb2 ], [ null, %bb1 ]
+  %5 = getelementptr i8, ptr %4, i64 24
+  %6 = icmp eq ptr %4, %i
+  br i1 %6, label %bb3, label %bb2
+
+bb3:
+  ret void
+}
+
 
 !llvm.module.flags = !{!0}
 !0 = !{i32 1, !"ProfileSummary", !1}

llvmbot · 2025-09-19T01:53:16Z

@llvm/pr-subscribers-llvm-transforms

Author: Aiden Grossman (boomanaiden154)

Changes

ControlHeightReduction will duplicate some blocks and insert phi nodes in exit blocks of regions that it operates on for any live values. This includes allocas. Having a lifetime annotation refer to a phi node was made illegal in 92c55a3, which causes the verifier to fail after CHR.

There are some cases where we might not need to drop lifetime annotations (usually because we do not need the phi to begin with), but drop all annotations for now to be conservative. I believe this also ends up being a correctness issue on top of the assertion because the lifetime annotation in some cases would get shifted to after the store, making the store UB when it was not before.

Fixes #159621.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp (+6)
(modified) llvm/test/Transforms/PGOProfile/chr.ll (+59)

diff --git a/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp b/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
index c14bbecf0d4e1..3e05b8539dcb8 100644
--- a/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
+++ b/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
@@ -1592,6 +1592,12 @@ static void insertTrivialPHIs(CHRScope *Scope,
         TrivialPHIs.insert(PN);
         CHR_DEBUG(dbgs() << "Insert phi " << *PN << "\n");
         for (Instruction *UI : Users) {
+          // Drop lifetime annotations as it is illegal for them to refer to a
+          // phi node.
+          if (UI->isLifetimeStartOrEnd()) {
+            UI->eraseFromParent();
+            continue;
+          }
           for (unsigned J = 0, NumOps = UI->getNumOperands(); J < NumOps; ++J) {
             if (UI->getOperand(J) == &I) {
               UI->setOperand(J, PN);
diff --git a/llvm/test/Transforms/PGOProfile/chr.ll b/llvm/test/Transforms/PGOProfile/chr.ll
index f0a1574c5f209..860e40917a77f 100644
--- a/llvm/test/Transforms/PGOProfile/chr.ll
+++ b/llvm/test/Transforms/PGOProfile/chr.ll
@@ -2678,6 +2678,65 @@ bb6:
   ret void
 }
 
+; Test that when we create phi nodes for allocas, we drop lifetime annotations
+; that would otherwise be illegally referring to a phi node.
+declare void @baz(i64)
+declare void @llvm.lifetime.start.p0(ptr captures(none))
+define void @test_chr_with_lifetimes(ptr %i) !prof !14 {
+; CHECK-LABEL: @test_chr_with_lifetimes(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[I:%.*]], align 4
+; CHECK-NEXT:    [[DOTFR:%.*]] = freeze i32 [[TMP0]]
+; CHECK-NEXT:    [[DOTNOT:%.*]] = icmp eq i32 [[DOTFR]], 0
+; CHECK-NEXT:    br i1 [[DOTNOT]], label [[ENTRY_SPLIT_NONCHR:%.*]], label [[ENTRY_SPLIT:%.*]], !prof [[PROF21:![0-9]+]]
+; CHECK:       entry.split:
+; CHECK-NEXT:    [[TEST:%.*]] = alloca i32, align 8
+; CHECK-NEXT:    call void @baz(i64 0)
+; CHECK-NEXT:    call void @foo()
+; CHECK-NEXT:    br label [[BB1:%.*]]
+; CHECK:       entry.split.nonchr:
+; CHECK-NEXT:    [[TEST_NONCHR:%.*]] = alloca i32, align 8
+; CHECK-NEXT:    call void @baz(i64 4)
+; CHECK-NEXT:    br label [[BB1]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[TMP1:%.*]] = phi ptr [ [[TEST]], [[ENTRY_SPLIT]] ], [ [[TEST_NONCHR]], [[ENTRY_SPLIT_NONCHR]] ]
+; CHECK-NEXT:    store ptr [[TMP1]], ptr [[I]], align 8
+; CHECK-NEXT:    br label [[BB2:%.*]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[TMP2:%.*]] = phi ptr [ [[TMP3:%.*]], [[BB2]] ], [ null, [[BB1]] ]
+; CHECK-NEXT:    [[TMP3]] = getelementptr i8, ptr [[TMP2]], i64 24
+; CHECK-NEXT:    [[TMP4:%.*]] = icmp eq ptr [[TMP2]], [[I]]
+; CHECK-NEXT:    br i1 [[TMP4]], label [[BB3:%.*]], label [[BB2]]
+; CHECK:       bb3:
+; CHECK-NEXT:    ret void
+;
+entry:
+  %1 = load i32, ptr %i
+  %2 = icmp eq i32 %1, 0
+  %3 = select i1 %2, i64 4, i64 0, !prof !15
+  %test = alloca i32, align 8
+  call void @baz(i64 %3)
+  br i1 %2, label %bb1, label %bb0, !prof !15
+
+bb0:
+  call void @foo()
+  br label %bb1
+
+bb1:
+  call void @llvm.lifetime.start.p0(ptr %test)
+  store ptr %test, ptr %i, align 8
+  br label %bb2
+
+bb2:
+  %4 = phi ptr [ %5, %bb2 ], [ null, %bb1 ]
+  %5 = getelementptr i8, ptr %4, i64 24
+  %6 = icmp eq ptr %4, %i
+  br i1 %6, label %bb3, label %bb2
+
+bb3:
+  ret void
+}
+
 
 !llvm.module.flags = !{!0}
 !0 = !{i32 1, !"ProfileSummary", !1}

nikic

I don't think this fix is quite right. For your example, CHR does something very undesirable, which is to split blocks in a way that ends up duplicating a previously static alloca, turning it into two dynamic allocas. We should really make sure that static allocas stay static. Additionally, I'm concerned that your approach here may end up partially removing lifetime markers. If we drop any markers, we always need to drop all of them.

The issue here looks very similar to the problem I fixed in fc90685. I think you basically want to do the same. That is, for static allocas, make sure they are not part of the splitting. And for dynamic allocas, drop all lifetime markers if any have to be dropped.

github-actions · 2025-09-19T19:25:22Z

✅ With the latest revision this PR passed the C/C++ code formatter.

boomanaiden154 · 2025-09-19T19:37:56Z

I don't think this fix is quite right. For your example, CHR does something very undesirable, which is to split blocks in a way that ends up duplicating a previously static alloca, turning it into two dynamic allocas. We should really make sure that static allocas stay static. Additionally, I'm concerned that your approach here may end up partially removing lifetime markers. If we drop any markers, we always need to drop all of them.

I've updated CHR to hoist static allocas to the true entry block. In that case we also don't need to generate a phi node and we can preserve the lifetime annotations. I've added another test case for dynamic allocas where we do need to remove lifetime annotations and updated insertTrivialPHIs to drop all lifetime annotations if we end up in a situation where we are trying to annotate the lifetime of a phi.

The issue here looks very similar to the problem I fixed in fc90685. I think you basically want to do the same. That is, for static allocas, make sure they are not part of the splitting. And for dynamic allocas, drop all lifetime markers if any have to be dropped.

Thanks for the example, that was super helpful. And thank you for the clear explanation/your patience as I get a better grip on the middle end.

…lvm#155957)" This reverts commit 8ed4899. This reapplies the original patch now that the issue in ControlHeightReduction (llvm#159621) has been fixed in llvm#159686.

nikic

LGTM, thanks!

#160133) …(#155957)" This reverts commit 8ed4899. This reapplies the original patch now that the issue in ControlHeightReduction (#159621) has been fixed in #159686.

…ssary (#159686)" This reverts commit a004509. Looks like this one is actually breaking the buildbots. Reverting the switch back to IRPGO did not fix things.

…essary (llvm#159686)" This reverts commit 4f33d7b. The original landing of this patch had an issue where it would try and hoist allocas into the entry block that were in the entry block. This would end up actually sinking them and moving them after users, resulting in invalid IR. This update fixes this by ensuring that we are only hoisting static allocas that have been sunk into a split basic block. A regression test has been added.

…essary" (#160640) Reapplies #159686 This reverts commit 4f33d7b. The original landing of this patch had an issue where it would try and hoist allocas into the entry block that were in the entry block. This would end up actually moving them lower in the block potentially after users, resulting in invalid IR. This update fixes this by ensuring that we are only hoisting static allocas that have been sunk into a split basic block. A regression test has been added. Integration tested using a three stage build of clang with IRPGO enabled.

…lvm#159686) ControlHeightReduction will duplicate some blocks and insert phi nodes in exit blocks of regions that it operates on for any live values. This includes allocas. Having a lifetime annotation refer to a phi node was made illegal in 92c55a3, which causes the verifier to fail after CHR. There are some cases where we might not need to drop lifetime annotations (usually because we do not need the phi to begin with), but drop all annotations for now to be conservative. Fixes llvm#159621.

llvm#160133) …(llvm#155957)" This reverts commit 8ed4899. This reapplies the original patch now that the issue in ControlHeightReduction (llvm#159621) has been fixed in llvm#159686.

…ssary (llvm#159686)" This reverts commit a004509. Looks like this one is actually breaking the buildbots. Reverting the switch back to IRPGO did not fix things.

…essary" (llvm#160640) Reapplies llvm#159686 This reverts commit 4f33d7b. The original landing of this patch had an issue where it would try and hoist allocas into the entry block that were in the entry block. This would end up actually moving them lower in the block potentially after users, resulting in invalid IR. This update fixes this by ensuring that we are only hoisting static allocas that have been sunk into a split basic block. A regression test has been added. Integration tested using a three stage build of clang with IRPGO enabled.

boomanaiden154 requested review from nikic, fhahn, ilovepi and efriedma-quic September 19, 2025 01:52

llvmbot added PGO Profile Guided Optimizations llvm:transforms labels Sep 19, 2025

nikic reviewed Sep 19, 2025

View reviewed changes

feedback part1

27fe33e

boomanaiden154 added 2 commits September 19, 2025 19:33

feedback part2

7cc7662

code formatting

e7fde50

boomanaiden154 requested a review from nikic September 19, 2025 19:37

boomanaiden154 mentioned this pull request Sep 22, 2025

Reapply "[Clang][CMake] Use IRPGO instead of FE PGO for Cmake Caches … #160133

Merged

Merge branch 'main' into fix-chr-lifetimes

e0f0dcb

nikic approved these changes Sep 22, 2025

View reviewed changes

boomanaiden154 merged commit a004509 into llvm:main Sep 22, 2025
8 of 9 checks passed

boomanaiden154 deleted the fix-chr-lifetimes branch September 22, 2025 20:58

boomanaiden154 mentioned this pull request Sep 25, 2025

Reapply "[ControlHeightReduction] Drop lifetime annotations where necessary" #160640

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ControlHeightReduction] Drop lifetime annotations where necessary #159686

[ControlHeightReduction] Drop lifetime annotations where necessary #159686

Uh oh!

boomanaiden154 commented Sep 19, 2025 •

edited

Loading

Uh oh!

llvmbot commented Sep 19, 2025

Uh oh!

llvmbot commented Sep 19, 2025

Uh oh!

nikic left a comment

Uh oh!

github-actions bot commented Sep 19, 2025 •

edited

Loading

Uh oh!

boomanaiden154 commented Sep 19, 2025 •

edited

Loading

Uh oh!

nikic left a comment

Uh oh!

Uh oh!

Uh oh!

[ControlHeightReduction] Drop lifetime annotations where necessary #159686

[ControlHeightReduction] Drop lifetime annotations where necessary #159686

Uh oh!

Conversation

boomanaiden154 commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 19, 2025

Uh oh!

llvmbot commented Sep 19, 2025

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boomanaiden154 commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

boomanaiden154 commented Sep 19, 2025 •

edited

Loading

github-actions bot commented Sep 19, 2025 •

edited

Loading

boomanaiden154 commented Sep 19, 2025 •

edited

Loading