[llvm] [MemorySSA] Don't create phi nodes in fixupDefs() (PR #156021)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 4 00:47:17 PDT 2025
https://github.com/nikic updated https://github.com/llvm/llvm-project/pull/156021
>From ea00722ef9be1d1b3d5a108f95c65b0bfd374e85 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Fri, 29 Aug 2025 14:47:38 +0200
Subject: [PATCH 1/2] [MemorySSA] Don't create phi nodes in fixupDefs()
The general flow when inserting MemoryDefs is:
* Insert the def and find it's defining access (may insert phis).
* Insert IDF phis
* Update defining access for defs after the inserted one (fixupDefs)
* Rename uses if requested
fixupDefs() uses getPreviousDef() which can create new MemoryPHIs,
but for which we're not going to insert IDF phis, so the required
dominance property may not hold.
I believe this is a leftover from a time before the "Insert IDF phis"
step existed. Now that step should ensure that all necessary
MemoryPhis have already been inserted, and we only need to update
them.
The fixupDefs() implementation was also returning after updating
a single access, which is not right.
---
llvm/lib/Analysis/MemorySSAUpdater.cpp | 22 ++-----
llvm/test/Transforms/LICM/pr117157.ll | 80 ++++++++++++++++++++++++++
2 files changed, 85 insertions(+), 17 deletions(-)
create mode 100644 llvm/test/Transforms/LICM/pr117157.ll
diff --git a/llvm/lib/Analysis/MemorySSAUpdater.cpp b/llvm/lib/Analysis/MemorySSAUpdater.cpp
index ecfecb03c3757..9f6bd55a87bd5 100644
--- a/llvm/lib/Analysis/MemorySSAUpdater.cpp
+++ b/llvm/lib/Analysis/MemorySSAUpdater.cpp
@@ -411,20 +411,11 @@ void MemorySSAUpdater::insertDef(MemoryDef *MD, bool RenameUses) {
FixupList.push_back(MD);
}
- // Remember the index where we stopped inserting new phis above, since the
- // fixupDefs call in the loop below may insert more, that are already minimal.
- unsigned NewPhiIndexEnd = InsertedPHIs.size();
-
- while (!FixupList.empty()) {
- unsigned StartingPHISize = InsertedPHIs.size();
- fixupDefs(FixupList);
- FixupList.clear();
- // Put any new phis on the fixup list, and process them
- FixupList.append(InsertedPHIs.begin() + StartingPHISize, InsertedPHIs.end());
- }
+ // Update defining access of following defs.
+ fixupDefs(FixupList);
// Optimize potentially non-minimal phis added in this method.
- unsigned NewPhiSize = NewPhiIndexEnd - NewPhiIndex;
+ unsigned NewPhiSize = InsertedPHIs.size() - NewPhiIndex;
if (NewPhiSize)
tryRemoveTrivialPhis(ArrayRef<WeakVH>(&InsertedPHIs[NewPhiIndex], NewPhiSize));
@@ -504,11 +495,8 @@ void MemorySSAUpdater::fixupDefs(const SmallVectorImpl<WeakVH> &Vars) {
assert(MSSA->dominates(NewDef, FirstDef) &&
"Should have dominated the new access");
- // This may insert new phi nodes, because we are not guaranteed the
- // block we are processing has a single pred, and depending where the
- // store was inserted, it may require phi nodes below it.
- cast<MemoryDef>(FirstDef)->setDefiningAccess(getPreviousDef(FirstDef));
- return;
+ cast<MemoryDef>(FirstDef)->setDefiningAccess(NewDef);
+ continue;
}
// We didn't find a def, so we must continue.
for (const auto *S : successors(FixupBlock)) {
diff --git a/llvm/test/Transforms/LICM/pr117157.ll b/llvm/test/Transforms/LICM/pr117157.ll
new file mode 100644
index 0000000000000..3c46e67bb5624
--- /dev/null
+++ b/llvm/test/Transforms/LICM/pr117157.ll
@@ -0,0 +1,80 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=licm -verify-memoryssa < %s | FileCheck %s
+
+define void @test(ptr %p) {
+; CHECK-LABEL: define void @test(
+; CHECK-SAME: ptr [[P:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: br label %[[LOOP0:.*]]
+; CHECK: [[LOOP0]]:
+; CHECK-NEXT: br label %[[LOOP1:.*]]
+; CHECK: [[LOOP1]]:
+; CHECK-NEXT: [[DEC10:%.*]] = phi i64 [ 0, %[[LOOP0]] ], [ 1, %[[LOOP1]] ]
+; CHECK-NEXT: br i1 false, label %[[LOOP1_EXIT:.*]], label %[[LOOP1]]
+; CHECK: [[LOOP1_EXIT]]:
+; CHECK-NEXT: [[DEC10_LCSSA:%.*]] = phi i64 [ [[DEC10]], %[[LOOP1]] ]
+; CHECK-NEXT: switch i32 0, label %[[LOOP0_LATCH:.*]] [
+; CHECK-NEXT: i32 0, label %[[LOOP0_LATCH]]
+; CHECK-NEXT: i32 2, label %[[LOOP3_PREHEADER:.*]]
+; CHECK-NEXT: i32 1, label %[[LOOP2:.*]]
+; CHECK-NEXT: ]
+; CHECK: [[LOOP2]]:
+; CHECK-NEXT: br i1 false, label %[[LOOP0_LATCH]], label %[[LOOP3_PREHEADER]]
+; CHECK: [[LOOP3_PREHEADER]]:
+; CHECK-NEXT: br label %[[LOOP3:.*]]
+; CHECK: [[LOOP3]]:
+; CHECK-NEXT: switch i32 0, label %[[EXIT:.*]] [
+; CHECK-NEXT: i32 0, label %[[LOOP3]]
+; CHECK-NEXT: i32 1, label %[[LOOP2_LATCH:.*]]
+; CHECK-NEXT: ]
+; CHECK: [[LOOP2_LATCH]]:
+; CHECK-NEXT: br label %[[LOOP2]]
+; CHECK: [[LOOP0_LATCH]]:
+; CHECK-NEXT: br label %[[LOOP0]]
+; CHECK: [[EXIT]]:
+; CHECK-NEXT: [[DEC10_LCSSA_LCSSA:%.*]] = phi i64 [ [[DEC10_LCSSA]], %[[LOOP3]] ]
+; CHECK-NEXT: store i64 [[DEC10_LCSSA_LCSSA]], ptr [[P]], align 4
+; CHECK-NEXT: store i64 1, ptr [[P]], align 4
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %loop0
+
+loop0: ; preds = %loop0.latch, %entry
+ br label %loop1
+
+loop1: ; preds = %loop1, %loop0
+ %dec10 = phi i64 [ 0, %loop0 ], [ 1, %loop1 ]
+ store i64 %dec10, ptr %p
+ br i1 false, label %loop1.exit, label %loop1
+
+loop1.exit: ; preds = %loop1
+ switch i32 0, label %loop0.latch [
+ i32 0, label %loop0.latch
+ i32 2, label %loop3.preheader
+ i32 1, label %loop2
+ ]
+
+loop2: ; preds = %loop2.latch, %loop1.exit
+ br i1 false, label %loop0.latch, label %loop3.preheader
+
+loop3.preheader: ; preds = %loop1.exit, %loop2
+ br label %loop3
+
+loop3: ; preds = %loop3.preheader, %loop3
+ switch i32 0, label %exit [
+ i32 0, label %loop3
+ i32 1, label %loop2.latch
+ ]
+
+loop2.latch: ; preds = %loop3
+ br label %loop2
+
+loop0.latch: ; preds = %loop2, %loop1.exit, %loop1.exit
+ store i64 0, ptr %p
+ br label %loop0
+
+exit: ; preds = %loop3
+ store i64 1, ptr %p
+ ret void
+}
>From 30cace773f7221074e67e57a5aa23fb07118be58 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Thu, 4 Sep 2025 09:46:54 +0200
Subject: [PATCH 2/2] Add assertion
---
llvm/lib/Analysis/MemorySSAUpdater.cpp | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/Analysis/MemorySSAUpdater.cpp b/llvm/lib/Analysis/MemorySSAUpdater.cpp
index 9f6bd55a87bd5..bb3e679219aea 100644
--- a/llvm/lib/Analysis/MemorySSAUpdater.cpp
+++ b/llvm/lib/Analysis/MemorySSAUpdater.cpp
@@ -412,10 +412,13 @@ void MemorySSAUpdater::insertDef(MemoryDef *MD, bool RenameUses) {
}
// Update defining access of following defs.
+ unsigned NewPhiIndexEnd = InsertedPHIs.size();
fixupDefs(FixupList);
+ assert(NewPhiIndexEnd == InsertedPHIs.size() &&
+ "Should not insert new phis during fixupDefs()");
// Optimize potentially non-minimal phis added in this method.
- unsigned NewPhiSize = InsertedPHIs.size() - NewPhiIndex;
+ unsigned NewPhiSize = NewPhiIndexEnd - NewPhiIndex;
if (NewPhiSize)
tryRemoveTrivialPhis(ArrayRef<WeakVH>(&InsertedPHIs[NewPhiIndex], NewPhiSize));
More information about the llvm-commits
mailing list