From b2eb1f4d3f536fe82812943ded40f8e94cb0be40 Mon Sep 17 00:00:00 2001 From: DianQK Date: Tue, 19 Nov 2024 22:01:12 +0800 Subject: [PATCH 1/7] Pre-commit test cases --- .../nontrivial-unswitch-pred-removed.ll | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll new file mode 100644 index 0000000000000..a5fa6c8f2acdf --- /dev/null +++ b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll @@ -0,0 +1,20 @@ +; RUN: opt -passes='loop-mssa(simple-loop-unswitch),print' -verify-memoryssa -disable-output -S < %s 2>&1 | FileCheck %s + +; CHECK: preds = %bb2{{$}} +; CHECK-NEXT: MemoryDef +; CHECK-NEXT: call i32 @bar() + +define i32 @foo(i1 %arg, ptr %arg1) { +bb: + br label %bb2 + +bb2: ; preds = %bb2, %bb + %i = select i1 %arg, ptr %arg1, ptr @bar + %i3 = call i32 %i() + br i1 %arg, label %bb2, label %bb4 + +bb4: ; preds = %bb2 + ret i32 %i3 +} + +declare i32 @bar() nounwind willreturn memory(none) From b164143bd0cf537cd17958628cda31acdeb86a8e Mon Sep 17 00:00:00 2001 From: DianQK Date: Tue, 19 Nov 2024 22:11:46 +0800 Subject: [PATCH 2/7] [SimpleLoopUnswitch] Preserve one PHI when removing a predecessor of a BB --- llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp | 2 +- .../nontrivial-unswitch-pred-removed.ll | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp index aa3cbc5e4bddc..d6fcf57f2b064 100644 --- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp +++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp @@ -1716,7 +1716,7 @@ static void deleteDeadBlocksFromLoop(Loop &L, auto *BB = DeathCandidates.pop_back_val(); if (!DeadBlockSet.count(BB) && !DT.isReachableFromEntry(BB)) { for (BasicBlock *SuccBB : successors(BB)) { - SuccBB->removePredecessor(BB); + SuccBB->removePredecessor(BB, /*KeepOneInputPHIs*/ true); DeathCandidates.push_back(SuccBB); } DeadBlockSet.insert(BB); diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll index a5fa6c8f2acdf..3545492f1e5e2 100644 --- a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll +++ b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll @@ -1,8 +1,12 @@ ; RUN: opt -passes='loop-mssa(simple-loop-unswitch),print' -verify-memoryssa -disable-output -S < %s 2>&1 | FileCheck %s +; RUN: opt -passes='loop-mssa(simple-loop-unswitch,licm)' -verify-memoryssa -disable-output -S -; CHECK: preds = %bb2{{$}} +; Check that SimpleLoopUnswitch preserves the MemoryDef of `call i32 @bar()` by preserving the PHI node. +; Also, check that executing LICM after SimpleLoopUnswitch does not result in a crash. + +; CHECK: %unswitched.select = phi ptr [ @bar, %bb2 ] ; CHECK-NEXT: MemoryDef -; CHECK-NEXT: call i32 @bar() +; CHECK-NEXT: call i32 %unswitched.select() define i32 @foo(i1 %arg, ptr %arg1) { bb: From 6b80bdb95627e4047c98e28991c503e3e90fddfa Mon Sep 17 00:00:00 2001 From: DianQK Date: Tue, 19 Nov 2024 22:21:25 +0800 Subject: [PATCH 3/7] [SimpleLoopUnswitch] Preserve one PHI when removing a predecessor of a cloned BB --- llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp index d6fcf57f2b064..d807c66bd9619 100644 --- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp +++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp @@ -1678,7 +1678,7 @@ deleteDeadClonedBlocks(Loop &L, ArrayRef ExitBlocks, if (BasicBlock *ClonedBB = cast_or_null(VMap->lookup(BB))) if (!DT.isReachableFromEntry(ClonedBB)) { for (BasicBlock *SuccBB : successors(ClonedBB)) - SuccBB->removePredecessor(ClonedBB); + SuccBB->removePredecessor(ClonedBB, /*KeepOneInputPHIs*/ true); DeadBlocks.push_back(ClonedBB); } From 5d2867be862430d5327571f1baeb01ba62fd6be8 Mon Sep 17 00:00:00 2001 From: DianQK Date: Wed, 20 Nov 2024 05:43:15 +0800 Subject: [PATCH 4/7] Revert "[SimpleLoopUnswitch] Preserve one PHI when removing a predecessor of a" This reverts commit 6b80bdb95627e4047c98e28991c503e3e90fddfa. --- llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp index d807c66bd9619..d6fcf57f2b064 100644 --- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp +++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp @@ -1678,7 +1678,7 @@ deleteDeadClonedBlocks(Loop &L, ArrayRef ExitBlocks, if (BasicBlock *ClonedBB = cast_or_null(VMap->lookup(BB))) if (!DT.isReachableFromEntry(ClonedBB)) { for (BasicBlock *SuccBB : successors(ClonedBB)) - SuccBB->removePredecessor(ClonedBB, /*KeepOneInputPHIs*/ true); + SuccBB->removePredecessor(ClonedBB); DeadBlocks.push_back(ClonedBB); } From e6b291a0a1d11c3937a3a99e827a9818848d051b Mon Sep 17 00:00:00 2001 From: DianQK Date: Wed, 20 Nov 2024 05:43:36 +0800 Subject: [PATCH 5/7] Revert "[SimpleLoopUnswitch] Preserve one PHI when removing a predecessor of a BB" This reverts commit b164143bd0cf537cd17958628cda31acdeb86a8e. --- llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp | 2 +- .../nontrivial-unswitch-pred-removed.ll | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp index d6fcf57f2b064..aa3cbc5e4bddc 100644 --- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp +++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp @@ -1716,7 +1716,7 @@ static void deleteDeadBlocksFromLoop(Loop &L, auto *BB = DeathCandidates.pop_back_val(); if (!DeadBlockSet.count(BB) && !DT.isReachableFromEntry(BB)) { for (BasicBlock *SuccBB : successors(BB)) { - SuccBB->removePredecessor(BB, /*KeepOneInputPHIs*/ true); + SuccBB->removePredecessor(BB); DeathCandidates.push_back(SuccBB); } DeadBlockSet.insert(BB); diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll index 3545492f1e5e2..a5fa6c8f2acdf 100644 --- a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll +++ b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll @@ -1,12 +1,8 @@ ; RUN: opt -passes='loop-mssa(simple-loop-unswitch),print' -verify-memoryssa -disable-output -S < %s 2>&1 | FileCheck %s -; RUN: opt -passes='loop-mssa(simple-loop-unswitch,licm)' -verify-memoryssa -disable-output -S -; Check that SimpleLoopUnswitch preserves the MemoryDef of `call i32 @bar()` by preserving the PHI node. -; Also, check that executing LICM after SimpleLoopUnswitch does not result in a crash. - -; CHECK: %unswitched.select = phi ptr [ @bar, %bb2 ] +; CHECK: preds = %bb2{{$}} ; CHECK-NEXT: MemoryDef -; CHECK-NEXT: call i32 %unswitched.select() +; CHECK-NEXT: call i32 @bar() define i32 @foo(i1 %arg, ptr %arg1) { bb: From e553a412bf217490fbeb13e93218b8d79aa81190 Mon Sep 17 00:00:00 2001 From: DianQK Date: Wed, 20 Nov 2024 05:57:15 +0800 Subject: [PATCH 6/7] [LICM] allow MemoryAccess creation failure --- llvm/include/llvm/Analysis/MemorySSAUpdater.h | 3 ++- llvm/lib/Analysis/MemorySSAUpdater.cpp | 8 +++++--- llvm/lib/Transforms/Scalar/LICM.cpp | 5 ++++- .../PR116813-memoryssa-outdated.ll} | 4 ++++ 4 files changed, 15 insertions(+), 5 deletions(-) rename llvm/test/Transforms/{SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll => LICM/PR116813-memoryssa-outdated.ll} (68%) diff --git a/llvm/include/llvm/Analysis/MemorySSAUpdater.h b/llvm/include/llvm/Analysis/MemorySSAUpdater.h index d4da3ef1146db..055feceefb054 100644 --- a/llvm/include/llvm/Analysis/MemorySSAUpdater.h +++ b/llvm/include/llvm/Analysis/MemorySSAUpdater.h @@ -190,7 +190,8 @@ class MemorySSAUpdater { /// inaccessible and it *must* have removeMemoryAccess called on it. MemoryAccess *createMemoryAccessInBB(Instruction *I, MemoryAccess *Definition, const BasicBlock *BB, - MemorySSA::InsertionPlace Point); + MemorySSA::InsertionPlace Point, + bool CreationMustSucceed = true); /// Create a MemoryAccess in MemorySSA before an existing MemoryAccess. /// diff --git a/llvm/lib/Analysis/MemorySSAUpdater.cpp b/llvm/lib/Analysis/MemorySSAUpdater.cpp index aa550f0b6a7bf..f672bd0e1e133 100644 --- a/llvm/lib/Analysis/MemorySSAUpdater.cpp +++ b/llvm/lib/Analysis/MemorySSAUpdater.cpp @@ -1403,9 +1403,11 @@ void MemorySSAUpdater::changeToUnreachable(const Instruction *I) { MemoryAccess *MemorySSAUpdater::createMemoryAccessInBB( Instruction *I, MemoryAccess *Definition, const BasicBlock *BB, - MemorySSA::InsertionPlace Point) { - MemoryUseOrDef *NewAccess = MSSA->createDefinedAccess(I, Definition); - MSSA->insertIntoListsForBlock(NewAccess, BB, Point); + MemorySSA::InsertionPlace Point, bool CreationMustSucceed) { + MemoryUseOrDef *NewAccess = MSSA->createDefinedAccess( + I, Definition, /*Template=*/nullptr, CreationMustSucceed); + if (NewAccess) + MSSA->insertIntoListsForBlock(NewAccess, BB, Point); return NewAccess; } diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp index fa04ced7182dc..94bfe44a847a3 100644 --- a/llvm/lib/Transforms/Scalar/LICM.cpp +++ b/llvm/lib/Transforms/Scalar/LICM.cpp @@ -1465,8 +1465,11 @@ static Instruction *cloneInstructionInExitBlock( if (MSSAU.getMemorySSA()->getMemoryAccess(&I)) { // Create a new MemoryAccess and let MemorySSA set its defining access. + // After running some passes, MemorySSA might be outdated, and the + // instruction `I` may have become a non-memory touching instruction. MemoryAccess *NewMemAcc = MSSAU.createMemoryAccessInBB( - New, nullptr, New->getParent(), MemorySSA::Beginning); + New, nullptr, New->getParent(), MemorySSA::Beginning, + /*CreationMustSucceed=*/false); if (NewMemAcc) { if (auto *MemDef = dyn_cast(NewMemAcc)) MSSAU.insertDef(MemDef, /*RenameUses=*/true); diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll b/llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll similarity index 68% rename from llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll rename to llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll index a5fa6c8f2acdf..50f2940a61dff 100644 --- a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll +++ b/llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll @@ -1,4 +1,8 @@ ; RUN: opt -passes='loop-mssa(simple-loop-unswitch),print' -verify-memoryssa -disable-output -S < %s 2>&1 | FileCheck %s +; RUN: opt -passes='loop-mssa(simple-loop-unswitch,licm)' -verify-memoryssa -disable-output -S + +; Check that SimpleLoopUnswitch preserves the MemoryDef of `call i32 @bar()`. +; Also, check that running LICM after SimpleLoopUnswitch does not result in a crash. ; CHECK: preds = %bb2{{$}} ; CHECK-NEXT: MemoryDef From de04782b7ee82623ab27e29688c43118b4c8e002 Mon Sep 17 00:00:00 2001 From: DianQK Date: Wed, 20 Nov 2024 18:38:34 +0800 Subject: [PATCH 7/7] Update test --- .../LICM/PR116813-memoryssa-outdated.ll | 52 ++++++++++++++----- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll b/llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll index 50f2940a61dff..a040c3cc6947c 100644 --- a/llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll +++ b/llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll @@ -1,23 +1,49 @@ -; RUN: opt -passes='loop-mssa(simple-loop-unswitch),print' -verify-memoryssa -disable-output -S < %s 2>&1 | FileCheck %s -; RUN: opt -passes='loop-mssa(simple-loop-unswitch,licm)' -verify-memoryssa -disable-output -S +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 +; RUN: opt -passes='loop-mssa(simple-loop-unswitch,licm)' -verify-memoryssa -S < %s | FileCheck %s -; Check that SimpleLoopUnswitch preserves the MemoryDef of `call i32 @bar()`. -; Also, check that running LICM after SimpleLoopUnswitch does not result in a crash. - -; CHECK: preds = %bb2{{$}} -; CHECK-NEXT: MemoryDef -; CHECK-NEXT: call i32 @bar() +; Check that running LICM after SimpleLoopUnswitch does not result in a crash. define i32 @foo(i1 %arg, ptr %arg1) { -bb: - br label %bb2 +; CHECK-LABEL: define i32 @foo( +; CHECK-SAME: i1 [[ARG:%.*]], ptr [[ARG1:%.*]]) { +; CHECK-NEXT: [[START:.*:]] +; CHECK-NEXT: [[ARG_FR:%.*]] = freeze i1 [[ARG]] +; CHECK-NEXT: br i1 [[ARG_FR]], label %[[START_SPLIT_US:.*]], label %[[START_SPLIT:.*]] +; CHECK: [[START_SPLIT_US]]: +; CHECK-NEXT: br label %[[LOOP_US:.*]] +; CHECK: [[LOOP_US]]: +; CHECK-NEXT: br label %[[BB0:.*]] +; CHECK: [[BB0]]: +; CHECK-NEXT: br label %[[BB1:.*]] +; CHECK: [[BB1]]: +; CHECK-NEXT: [[UNSWITCHED_SELECT_US:%.*]] = phi ptr [ [[ARG1]], %[[BB0]] ] +; CHECK-NEXT: [[I3_US:%.*]] = call i32 [[UNSWITCHED_SELECT_US]]() +; CHECK-NEXT: br i1 true, label %[[LOOP_US]], label %[[RET_SPLIT_US:.*]] +; CHECK: [[RET_SPLIT_US]]: +; CHECK-NEXT: [[I3_LCSSA_US:%.*]] = phi i32 [ [[I3_US]], %[[BB1]] ] +; CHECK-NEXT: br label %[[RET:.*]] +; CHECK: [[START_SPLIT]]: +; CHECK-NEXT: br label %[[LOOP:.*]] +; CHECK: [[LOOP]]: +; CHECK-NEXT: br label %[[BB2:.*]] +; CHECK: [[BB2]]: +; CHECK-NEXT: br i1 false, label %[[LOOP]], label %[[RET_SPLIT:.*]] +; CHECK: [[RET_SPLIT]]: +; CHECK-NEXT: [[I3_LE:%.*]] = call i32 @bar() +; CHECK-NEXT: br label %[[RET]] +; CHECK: [[RET]]: +; CHECK-NEXT: [[DOTUS_PHI:%.*]] = phi i32 [ [[I3_LE]], %[[RET_SPLIT]] ], [ [[I3_LCSSA_US]], %[[RET_SPLIT_US]] ] +; CHECK-NEXT: ret i32 [[DOTUS_PHI]] +; +start: + br label %loop -bb2: ; preds = %bb2, %bb +loop: ; preds = %loop, %bb %i = select i1 %arg, ptr %arg1, ptr @bar %i3 = call i32 %i() - br i1 %arg, label %bb2, label %bb4 + br i1 %arg, label %loop, label %ret -bb4: ; preds = %bb2 +ret: ; preds = %loop ret i32 %i3 }