From 562addba652e8bdabe49f9123fd92c21b7a0d640 Mon Sep 17 00:00:00 2001 From: Jamie Schmeiser Date: Wed, 18 Nov 2020 15:17:53 -0500 Subject: [PATCH] Revert "Expand existing loopsink testing to also test loopsinking using new pass manager and fix LICM bug." This reverts commit d4ba28bddc89a14885218b9eaa4fbf6654c2a5bd. --- llvm/lib/Transforms/Scalar/LICM.cpp | 33 ++--- llvm/lib/Transforms/Scalar/LoopSink.cpp | 129 +++--------------- llvm/test/Transforms/LICM/loopsink-pr38462.ll | 3 - llvm/test/Transforms/LICM/loopsink-pr39570.ll | 3 - llvm/test/Transforms/LICM/loopsink-pr39695.ll | 3 - llvm/test/Transforms/LICM/loopsink.ll | 2 - 6 files changed, 32 insertions(+), 141 deletions(-) diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp index 1885d1d9f557..e64ec4bf6671 100644 --- a/llvm/lib/Transforms/Scalar/LICM.cpp +++ b/llvm/lib/Transforms/Scalar/LICM.cpp @@ -163,10 +163,8 @@ static bool pointerInvalidatedByLoop(MemoryLocation MemLoc, AliasSetTracker *CurAST, Loop *CurLoop, AAResults *AA); static bool pointerInvalidatedByLoopWithMSSA(MemorySSA *MSSA, MemoryUse *MU, - Loop *CurLoop, Instruction &I, + Loop *CurLoop, SinkAndHoistLICMFlags &Flags); -static bool pointerInvalidatedByBlockWithMSSA(BasicBlock &BB, MemorySSA &MSSA, - MemoryUse &MU); static Instruction *cloneInstructionInExitBlock( Instruction &I, BasicBlock &ExitBlock, PHINode &PN, const LoopInfo *LI, const LoopSafetyInfo *SafetyInfo, MemorySSAUpdater *MSSAU); @@ -1157,7 +1155,7 @@ bool llvm::canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT, CurLoop, AA); else Invalidated = pointerInvalidatedByLoopWithMSSA( - MSSA, cast(MSSA->getMemoryAccess(LI)), CurLoop, I, *Flags); + MSSA, cast(MSSA->getMemoryAccess(LI)), CurLoop, *Flags); // Check loop-invariant address because this may also be a sinkable load // whose address is not necessarily loop-invariant. if (ORE && Invalidated && CurLoop->isLoopInvariant(LI->getPointerOperand())) @@ -1213,7 +1211,7 @@ bool llvm::canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT, CurAST, CurLoop, AA); else Invalidated = pointerInvalidatedByLoopWithMSSA( - MSSA, cast(MSSA->getMemoryAccess(CI)), CurLoop, I, + MSSA, cast(MSSA->getMemoryAccess(CI)), CurLoop, *Flags); if (Invalidated) return false; @@ -1314,6 +1312,7 @@ bool llvm::canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT, } } } + auto *Source = MSSA->getSkipSelfWalker()->getClobberingMemoryAccess(SI); Flags->incrementClobberingCalls(); // If there are no clobbering Defs in the loop, store is safe to hoist. @@ -2286,7 +2285,7 @@ static bool pointerInvalidatedByLoop(MemoryLocation MemLoc, } bool pointerInvalidatedByLoopWithMSSA(MemorySSA *MSSA, MemoryUse *MU, - Loop *CurLoop, Instruction &I, + Loop *CurLoop, SinkAndHoistLICMFlags &Flags) { // For hoisting, use the walker to determine safety if (!Flags.getIsSink()) { @@ -2322,22 +2321,12 @@ bool pointerInvalidatedByLoopWithMSSA(MemorySSA *MSSA, MemoryUse *MU, if (Flags.tooManyMemoryAccesses()) return true; for (auto *BB : CurLoop->getBlocks()) - if (pointerInvalidatedByBlockWithMSSA(*BB, *MSSA, *MU)) - return true; - // When sinking, the source block may not be part of the loop so check it. - if (!CurLoop->contains(&I)) - return pointerInvalidatedByBlockWithMSSA(*I.getParent(), *MSSA, *MU); - - return false; -} - -bool pointerInvalidatedByBlockWithMSSA(BasicBlock &BB, MemorySSA &MSSA, - MemoryUse &MU) { - if (const auto *Accesses = MSSA.getBlockDefs(&BB)) - for (const auto &MA : *Accesses) - if (const auto *MD = dyn_cast(&MA)) - if (MU.getBlock() != MD->getBlock() || !MSSA.locallyDominates(MD, &MU)) - return true; + if (auto *Accesses = MSSA->getBlockDefs(BB)) + for (const auto &MA : *Accesses) + if (const auto *MD = dyn_cast(&MA)) + if (MU->getBlock() != MD->getBlock() || + !MSSA->locallyDominates(MD, MU)) + return true; return false; } diff --git a/llvm/lib/Transforms/Scalar/LoopSink.cpp b/llvm/lib/Transforms/Scalar/LoopSink.cpp index 6b9333ce5c5d..1c03a4bf6c02 100644 --- a/llvm/lib/Transforms/Scalar/LoopSink.cpp +++ b/llvm/lib/Transforms/Scalar/LoopSink.cpp @@ -39,8 +39,6 @@ #include "llvm/Analysis/Loads.h" #include "llvm/Analysis/LoopInfo.h" #include "llvm/Analysis/LoopPass.h" -#include "llvm/Analysis/MemorySSA.h" -#include "llvm/Analysis/MemorySSAUpdater.h" #include "llvm/Analysis/ScalarEvolution.h" #include "llvm/Analysis/ScalarEvolutionAliasAnalysis.h" #include "llvm/IR/Dominators.h" @@ -69,14 +67,6 @@ static cl::opt MaxNumberOfUseBBsForSinking( "max-uses-for-sinking", cl::Hidden, cl::init(30), cl::desc("Do not sink instructions that have too many uses.")); -static cl::opt EnableMSSAInLoopSink( - "enable-mssa-in-loop-sink", cl::Hidden, cl::init(false), - cl::desc("Enable MemorySSA for LoopSink in new pass manager")); - -static cl::opt EnableMSSAInLegacyLoopSink( - "enable-mssa-in-legacy-loop-sink", cl::Hidden, cl::init(false), - cl::desc("Enable MemorySSA for LoopSink in legacy pass manager")); - /// Return adjusted total frequency of \p BBs. /// /// * If there is only one BB, sinking instruction will not introduce code @@ -182,10 +172,11 @@ findBBsToSinkInto(const Loop &L, const SmallPtrSetImpl &UseBBs, // sinking is successful. // \p LoopBlockNumber is used to sort the insertion blocks to ensure // determinism. -static bool sinkInstruction( - Loop &L, Instruction &I, const SmallVectorImpl &ColdLoopBBs, - const SmallDenseMap &LoopBlockNumber, LoopInfo &LI, - DominatorTree &DT, BlockFrequencyInfo &BFI, MemorySSAUpdater *MSSAU) { +static bool sinkInstruction(Loop &L, Instruction &I, + const SmallVectorImpl &ColdLoopBBs, + const SmallDenseMap &LoopBlockNumber, + LoopInfo &LI, DominatorTree &DT, + BlockFrequencyInfo &BFI) { // Compute the set of blocks in loop L which contain a use of I. SmallPtrSet BBs; for (auto &U : I.uses()) { @@ -239,21 +230,6 @@ static bool sinkInstruction( Instruction *IC = I.clone(); IC->setName(I.getName()); IC->insertBefore(&*N->getFirstInsertionPt()); - - if (MSSAU && MSSAU->getMemorySSA()->getMemoryAccess(&I)) { - // Create a new MemoryAccess and let MemorySSA set its defining access. - MemoryAccess *NewMemAcc = - MSSAU->createMemoryAccessInBB(IC, nullptr, N, MemorySSA::Beginning); - if (NewMemAcc) { - if (auto *MemDef = dyn_cast(NewMemAcc)) - MSSAU->insertDef(MemDef, /*RenameUses=*/true); - else { - auto *MemUse = cast(NewMemAcc); - MSSAU->insertUse(MemUse, /*RenameUses=*/true); - } - } - } - // Replaces uses of I with IC in N I.replaceUsesWithIf(IC, [N](Use &U) { return cast(U.getUser())->getParent() == N; @@ -268,11 +244,6 @@ static bool sinkInstruction( NumLoopSunk++; I.moveBefore(&*MoveBB->getFirstInsertionPt()); - if (MSSAU) - if (MemoryUseOrDef *OldMemAcc = cast_or_null( - MSSAU->getMemorySSA()->getMemoryAccess(&I))) - MSSAU->moveToPlace(OldMemAcc, MoveBB, MemorySSA::Beginning); - return true; } @@ -281,11 +252,10 @@ static bool sinkInstruction( static bool sinkLoopInvariantInstructions(Loop &L, AAResults &AA, LoopInfo &LI, DominatorTree &DT, BlockFrequencyInfo &BFI, - ScalarEvolution *SE, - AliasSetTracker *CurAST, - MemorySSA *MSSA) { + ScalarEvolution *SE) { BasicBlock *Preheader = L.getLoopPreheader(); - assert(Preheader && "Expected loop to have preheader"); + if (!Preheader) + return false; // Enable LoopSink only when runtime profile is available. // With static profile, the sinking decision may be sub-optimal. @@ -301,15 +271,13 @@ static bool sinkLoopInvariantInstructions(Loop &L, AAResults &AA, LoopInfo &LI, })) return false; - std::unique_ptr MSSAU; - std::unique_ptr LICMFlags; - if (MSSA) { - MSSAU = std::make_unique(MSSA); - LICMFlags = - std::make_unique(/*IsSink=*/true, &L, MSSA); - } - bool Changed = false; + AliasSetTracker CurAST(AA); + + // Compute alias set. + for (BasicBlock *BB : L.blocks()) + CurAST.add(*BB); + CurAST.add(*Preheader); // Sort loop's basic blocks by frequency SmallVector ColdLoopBBs; @@ -332,11 +300,9 @@ static bool sinkLoopInvariantInstructions(Loop &L, AAResults &AA, LoopInfo &LI, // No need to check for instruction's operands are loop invariant. assert(L.hasLoopInvariantOperands(I) && "Insts in a loop's preheader should have loop invariant operands!"); - if (!canSinkOrHoistInst(*I, &AA, &DT, &L, CurAST, MSSAU.get(), false, - LICMFlags.get())) + if (!canSinkOrHoistInst(*I, &AA, &DT, &L, &CurAST, nullptr, false)) continue; - if (sinkInstruction(L, *I, ColdLoopBBs, LoopBlockNumber, LI, DT, BFI, - MSSAU.get())) + if (sinkInstruction(L, *I, ColdLoopBBs, LoopBlockNumber, LI, DT, BFI)) Changed = true; } @@ -345,13 +311,6 @@ static bool sinkLoopInvariantInstructions(Loop &L, AAResults &AA, LoopInfo &LI, return Changed; } -static void computeAliasSet(Loop &L, BasicBlock &Preheader, - AliasSetTracker &CurAST) { - for (BasicBlock *BB : L.blocks()) - CurAST.add(*BB); - CurAST.add(Preheader); -} - PreservedAnalyses LoopSinkPass::run(Function &F, FunctionAnalysisManager &FAM) { LoopInfo &LI = FAM.getResult(F); // Nothing to do if there are no loops. @@ -362,10 +321,6 @@ PreservedAnalyses LoopSinkPass::run(Function &F, FunctionAnalysisManager &FAM) { DominatorTree &DT = FAM.getResult(F); BlockFrequencyInfo &BFI = FAM.getResult(F); - MemorySSA *MSSA = EnableMSSAInLoopSink - ? &FAM.getResult(F).getMSSA() - : nullptr; - // We want to do a postorder walk over the loops. Since loops are a tree this // is equivalent to a reversed preorder walk and preorder is easy to compute // without recursion. Since we reverse the preorder, we will visit siblings @@ -377,22 +332,11 @@ PreservedAnalyses LoopSinkPass::run(Function &F, FunctionAnalysisManager &FAM) { do { Loop &L = *PreorderLoops.pop_back_val(); - BasicBlock *Preheader = L.getLoopPreheader(); - if (!Preheader) - continue; - - std::unique_ptr CurAST; - if (!EnableMSSAInLoopSink) { - CurAST = std::make_unique(AA); - computeAliasSet(L, *Preheader, *CurAST.get()); - } - // Note that we don't pass SCEV here because it is only used to invalidate // loops in SCEV and we don't preserve (or request) SCEV at all making that // unnecessary. Changed |= sinkLoopInvariantInstructions(L, AA, LI, DT, BFI, - /*ScalarEvolution*/ nullptr, - CurAST.get(), MSSA); + /*ScalarEvolution*/ nullptr); } while (!PreorderLoops.empty()); if (!Changed) @@ -400,14 +344,6 @@ PreservedAnalyses LoopSinkPass::run(Function &F, FunctionAnalysisManager &FAM) { PreservedAnalyses PA; PA.preserveSet(); - - if (MSSA) { - PA.preserve(); - - if (VerifyMemorySSA) - MSSA->verifyMemorySSA(); - } - return PA; } @@ -422,41 +358,19 @@ struct LegacyLoopSinkPass : public LoopPass { if (skipLoop(L)) return false; - BasicBlock *Preheader = L->getLoopPreheader(); - if (!Preheader) - return false; - - AAResults &AA = getAnalysis().getAAResults(); auto *SE = getAnalysisIfAvailable(); - std::unique_ptr CurAST; - MemorySSA *MSSA = nullptr; - if (EnableMSSAInLegacyLoopSink) - MSSA = &getAnalysis().getMSSA(); - else { - CurAST = std::make_unique(AA); - computeAliasSet(*L, *Preheader, *CurAST.get()); - } - - bool Changed = sinkLoopInvariantInstructions( - *L, AA, getAnalysis().getLoopInfo(), + return sinkLoopInvariantInstructions( + *L, getAnalysis().getAAResults(), + getAnalysis().getLoopInfo(), getAnalysis().getDomTree(), getAnalysis().getBFI(), - SE ? &SE->getSE() : nullptr, CurAST.get(), MSSA); - - if (MSSA && VerifyMemorySSA) - MSSA->verifyMemorySSA(); - - return Changed; + SE ? &SE->getSE() : nullptr); } void getAnalysisUsage(AnalysisUsage &AU) const override { AU.setPreservesCFG(); AU.addRequired(); getLoopAnalysisUsage(AU); - if (EnableMSSAInLegacyLoopSink) { - AU.addRequired(); - AU.addPreserved(); - } } }; } @@ -466,7 +380,6 @@ INITIALIZE_PASS_BEGIN(LegacyLoopSinkPass, "loop-sink", "Loop Sink", false, false) INITIALIZE_PASS_DEPENDENCY(LoopPass) INITIALIZE_PASS_DEPENDENCY(BlockFrequencyInfoWrapperPass) -INITIALIZE_PASS_DEPENDENCY(MemorySSAWrapperPass) INITIALIZE_PASS_END(LegacyLoopSinkPass, "loop-sink", "Loop Sink", false, false) Pass *llvm::createLoopSinkPass() { return new LegacyLoopSinkPass(); } diff --git a/llvm/test/Transforms/LICM/loopsink-pr38462.ll b/llvm/test/Transforms/LICM/loopsink-pr38462.ll index 3dd6397bf5ba..146e2506b7eb 100644 --- a/llvm/test/Transforms/LICM/loopsink-pr38462.ll +++ b/llvm/test/Transforms/LICM/loopsink-pr38462.ll @@ -1,7 +1,4 @@ ; RUN: opt -S -loop-sink < %s | FileCheck %s -; RUN: opt -S -verify-memoryssa -enable-mssa-in-legacy-loop-sink -loop-sink < %s | FileCheck %s -; RUN: opt -S -aa-pipeline=basic-aa -passes=loop-sink < %s | FileCheck %s -; RUN: opt -S -verify-memoryssa -enable-mssa-in-loop-sink -aa-pipeline=basic-aa -passes=loop-sink < %s | FileCheck %s target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-pc-windows-msvc19.13.26128" diff --git a/llvm/test/Transforms/LICM/loopsink-pr39570.ll b/llvm/test/Transforms/LICM/loopsink-pr39570.ll index 893331fc7ce4..65d3e1f51395 100644 --- a/llvm/test/Transforms/LICM/loopsink-pr39570.ll +++ b/llvm/test/Transforms/LICM/loopsink-pr39570.ll @@ -1,7 +1,4 @@ ; RUN: opt -S -loop-sink < %s | FileCheck %s -; RUN: opt -S -verify-memoryssa -enable-mssa-in-legacy-loop-sink -loop-sink < %s | FileCheck %s -; RUN: opt -S -aa-pipeline=basic-aa -passes=loop-sink < %s | FileCheck %s -; RUN: opt -S -verify-memoryssa -enable-mssa-in-loop-sink -aa-pipeline=basic-aa -passes=loop-sink < %s | FileCheck %s ; CHECK: pr39570 ; Make sure not to assert. diff --git a/llvm/test/Transforms/LICM/loopsink-pr39695.ll b/llvm/test/Transforms/LICM/loopsink-pr39695.ll index 71c36dc2259d..4d33210352de 100644 --- a/llvm/test/Transforms/LICM/loopsink-pr39695.ll +++ b/llvm/test/Transforms/LICM/loopsink-pr39695.ll @@ -1,7 +1,4 @@ ; RUN: opt -S -loop-sink < %s | FileCheck %s -; RUN: opt -S -verify-memoryssa -enable-mssa-in-legacy-loop-sink -loop-sink < %s | FileCheck %s -; RUN: opt -S -aa-pipeline=basic-aa -passes=loop-sink < %s | FileCheck %s -; RUN: opt -S -verify-memoryssa -enable-mssa-in-loop-sink -aa-pipeline=basic-aa -passes=loop-sink < %s | FileCheck %s ; The load instruction should not be sunk into following loop. ; CHECK: @foo diff --git a/llvm/test/Transforms/LICM/loopsink.ll b/llvm/test/Transforms/LICM/loopsink.ll index 7982fd2957f2..f9bdbae06cd4 100644 --- a/llvm/test/Transforms/LICM/loopsink.ll +++ b/llvm/test/Transforms/LICM/loopsink.ll @@ -1,7 +1,5 @@ ; RUN: opt -S -loop-sink < %s | FileCheck %s -; RUN: opt -S -verify-memoryssa -enable-mssa-in-legacy-loop-sink -loop-sink < %s | FileCheck %s ; RUN: opt -S -aa-pipeline=basic-aa -passes=loop-sink < %s | FileCheck %s -; RUN: opt -S -verify-memoryssa -enable-mssa-in-loop-sink -aa-pipeline=basic-aa -passes=loop-sink < %s | FileCheck %s @g = global i32 0, align 4