From c4d8c6319f576a7540017168db2f0440691914f4 Mon Sep 17 00:00:00 2001 From: Daniil Suchkov Date: Thu, 21 Nov 2019 17:56:02 +0700 Subject: [PATCH] [LCSSA] Don't use VH callbacks to invalidate SCEV when creating LCSSA phis In general ValueHandleBase::ValueIsRAUWd shouldn't be called when not all uses of the value were actually replaced, though, currently formLCSSAForInstructions calls it when it inserts LCSSA-phis. Calls of ValueHandleBase::ValueIsRAUWd were added to LCSSA specifically to update/invalidate SCEV. In the best case these calls duplicate some of the work already done by SE->forgetValue, though in case when SCEV of the value is SCEVUnknown, SCEV replaces the underlying value of SCEVUnknown with the new value (i.e. acts like LCSSA-phi actually fully replaces the value it is created for), which leads to SCEV being corrupted because LCSSA-phi rarely dominates all uses of its inputs. Fixes bug https://bugs.llvm.org/show_bug.cgi?id=44058. Reviewers: fhahn, efriedma, reames, sanjoy.google Reviewed By: fhahn Subscribers: hiraditya, javed.absar, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D70593 --- .../Transforms/Scalar/SimpleLoopUnswitch.cpp | 10 ++--- llvm/lib/Transforms/Utils/LCSSA.cpp | 7 ---- llvm/test/Transforms/LCSSA/pr44058.ll | 37 +++++++++++++++++++ 3 files changed, 42 insertions(+), 12 deletions(-) create mode 100644 llvm/test/Transforms/LCSSA/pr44058.ll diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp index d441c6bbf124..d7a34acb4318 100644 --- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp +++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp @@ -265,7 +265,7 @@ static void rewritePHINodesForExitAndUnswitchedBlocks(BasicBlock &ExitBB, /// to an entirely separate nest. static void hoistLoopToNewParent(Loop &L, BasicBlock &Preheader, DominatorTree &DT, LoopInfo &LI, - MemorySSAUpdater *MSSAU) { + MemorySSAUpdater *MSSAU, ScalarEvolution *SE) { // If the loop is already at the top level, we can't hoist it anywhere. Loop *OldParentL = L.getParentLoop(); if (!OldParentL) @@ -319,7 +319,7 @@ static void hoistLoopToNewParent(Loop &L, BasicBlock &Preheader, // Because we just hoisted a loop out of this one, we have essentially // created new exit paths from it. That means we need to form LCSSA PHI // nodes for values used in the no-longer-nested loop. - formLCSSA(*OldContainingL, DT, &LI, nullptr); + formLCSSA(*OldContainingL, DT, &LI, SE); // We shouldn't need to form dedicated exits because the exit introduced // here is the (just split by unswitching) preheader. However, after trivial @@ -549,7 +549,7 @@ static bool unswitchTrivialBranch(Loop &L, BranchInst &BI, DominatorTree &DT, // If this was full unswitching, we may have changed the nesting relationship // for this loop so hoist it to its correct parent if needed. if (FullUnswitch) - hoistLoopToNewParent(L, *NewPH, DT, LI, MSSAU); + hoistLoopToNewParent(L, *NewPH, DT, LI, MSSAU, SE); if (MSSAU && VerifyMemorySSA) MSSAU->getMemorySSA()->verifyMemorySSA(); @@ -842,7 +842,7 @@ static bool unswitchTrivialSwitch(Loop &L, SwitchInst &SI, DominatorTree &DT, // We may have changed the nesting relationship for this loop so hoist it to // its correct parent if needed. - hoistLoopToNewParent(L, *NewPH, DT, LI, MSSAU); + hoistLoopToNewParent(L, *NewPH, DT, LI, MSSAU, SE); if (MSSAU && VerifyMemorySSA) MSSAU->getMemorySSA()->verifyMemorySSA(); @@ -2277,7 +2277,7 @@ static void unswitchNontrivialInvariants( // First build LCSSA for this loop so that we can preserve it when // forming dedicated exits. We don't want to perturb some other loop's // LCSSA while doing that CFG edit. - formLCSSA(UpdateL, DT, &LI, nullptr); + formLCSSA(UpdateL, DT, &LI, SE); // For loops reached by this loop's original exit blocks we may // introduced new, non-dedicated exits. At least try to re-form dedicated diff --git a/llvm/lib/Transforms/Utils/LCSSA.cpp b/llvm/lib/Transforms/Utils/LCSSA.cpp index f5946a3d99f7..5746d69260d5 100644 --- a/llvm/lib/Transforms/Utils/LCSSA.cpp +++ b/llvm/lib/Transforms/Utils/LCSSA.cpp @@ -200,9 +200,6 @@ bool llvm::formLCSSAForInstructions(SmallVectorImpl &Worklist, UserBB = PN->getIncomingBlock(*UseToRewrite); if (isa(UserBB->begin()) && isExitBlock(UserBB, ExitBlocks)) { - // Tell the VHs that the uses changed. This updates SCEV's caches. - if (UseToRewrite->get()->hasValueHandle()) - ValueHandleBase::ValueIsRAUWd(*UseToRewrite, &UserBB->front()); UseToRewrite->set(&UserBB->front()); continue; } @@ -210,10 +207,6 @@ bool llvm::formLCSSAForInstructions(SmallVectorImpl &Worklist, // If we added a single PHI, it must dominate all uses and we can directly // rename it. if (AddedPHIs.size() == 1) { - // Tell the VHs that the uses changed. This updates SCEV's caches. - // We might call ValueIsRAUWd multiple times for the same value. - if (UseToRewrite->get()->hasValueHandle()) - ValueHandleBase::ValueIsRAUWd(*UseToRewrite, AddedPHIs[0]); UseToRewrite->set(AddedPHIs[0]); continue; } diff --git a/llvm/test/Transforms/LCSSA/pr44058.ll b/llvm/test/Transforms/LCSSA/pr44058.ll new file mode 100644 index 000000000000..b6fa92ab7cb2 --- /dev/null +++ b/llvm/test/Transforms/LCSSA/pr44058.ll @@ -0,0 +1,37 @@ +; RUN: opt -passes="verify,lcssa,verify" -verify-scev-strict -S %s + +; The first SCEV verification is required because it queries SCEV and populates +; SCEV caches. Second SCEV verification checks if the caches are in valid state. + +; Check that the second SCEV verification doesn't fail. +define void @foo(i32* %arg, i32* %arg1, i1 %arg2) { +bb: + br label %bb3 + +bb3: ; preds = %bb13, %bb + %tmp = load i32, i32* %arg + %tmp4 = load i32, i32* %arg1 + %tmp5 = add i32 %tmp4, %tmp + %tmp6 = icmp sgt i32 %tmp5, %tmp + br i1 %tmp6, label %bb7, label %bb11 + +bb7: ; preds = %bb3 + br i1 %arg2, label %bb10, label %bb8 + +bb8: ; preds = %bb7 + %tmp9 = add nsw i32 %tmp, 1 + ret void + +bb10: ; preds = %bb7 + br label %bb11 + +bb11: ; preds = %bb10, %bb3 + %tmp12 = phi i32 [ 0, %bb3 ], [ %tmp4, %bb10 ] + br label %bb13 + +bb13: ; preds = %bb13, %bb11 + %tmp14 = phi i32 [ %tmp15, %bb13 ], [ 0, %bb11 ] + %tmp15 = add nuw nsw i32 %tmp14, 1 + %tmp16 = icmp slt i32 %tmp15, %tmp12 + br i1 %tmp16, label %bb13, label %bb3 +}