From 6a7bbfb2e26ce9a39a69fccff8383ba7f484e2e2 Mon Sep 17 00:00:00 2001 From: Philip Reames Date: Tue, 17 Oct 2017 06:21:07 +0000 Subject: [PATCH] Revert 315440 on behalf of mkazantsev This patch reverts rL315440 because of the bug described at https://bugs.llvm.org/show_bug.cgi?id=34937 The fix for the bug is on review as D38944, but not yet ready. Given this is a regression reverting until a fix is ready is called for. Max would have done the revert himself, but is having trouble doing a build of fresh LLVM for some reason. I did the build and test to ensure the revert worked as expected on his behalf. llvm-svn: 315974 --- llvm/include/llvm/Transforms/Scalar/GVN.h | 7 --- llvm/lib/Transforms/Scalar/GVN.cpp | 77 ------------------------------- llvm/test/Transforms/GVN/PRE/pre-load.ll | 28 ----------- 3 files changed, 112 deletions(-) diff --git a/llvm/include/llvm/Transforms/Scalar/GVN.h b/llvm/include/llvm/Transforms/Scalar/GVN.h index 08db933..251492a 100644 --- a/llvm/include/llvm/Transforms/Scalar/GVN.h +++ b/llvm/include/llvm/Transforms/Scalar/GVN.h @@ -18,7 +18,6 @@ #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/MapVector.h" -#include "llvm/ADT/PostOrderIterator.h" #include "llvm/ADT/SetVector.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Analysis/AliasAnalysis.h" @@ -28,7 +27,6 @@ #include "llvm/IR/PassManager.h" #include "llvm/Support/Allocator.h" #include "llvm/Support/Compiler.h" -#include "llvm/Transforms/Utils/OrderedInstructions.h" #include #include #include @@ -158,11 +156,7 @@ private: AssumptionCache *AC; SetVector DeadBlocks; OptimizationRemarkEmitter *ORE; - // Maps a block to the topmost instruction with implicit control flow in it. - DenseMap - FirstImplicitControlFlowInsts; - OrderedInstructions *OI; ValueTable VN; /// A mapping from value numbers to lists of Value*'s that @@ -274,7 +268,6 @@ private: BasicBlock *Curr, unsigned int ValNo); Value *findLeader(const BasicBlock *BB, uint32_t num); void cleanupGlobalSets(); - void fillImplicitControlFlowInfo(ReversePostOrderTraversal &RPOT); void verifyRemoved(const Instruction *I) const; bool splitCriticalEdges(); BasicBlock *splitCriticalEdges(BasicBlock *Pred, BasicBlock *Succ); diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp index a642eec..9d2c7b9 100644 --- a/llvm/lib/Transforms/Scalar/GVN.cpp +++ b/llvm/lib/Transforms/Scalar/GVN.cpp @@ -38,7 +38,6 @@ #include "llvm/Analysis/OptimizationRemarkEmitter.h" #include "llvm/Analysis/PHITransAddr.h" #include "llvm/Analysis/TargetLibraryInfo.h" -#include "llvm/Analysis/ValueTracking.h" #include "llvm/IR/Attributes.h" #include "llvm/IR/BasicBlock.h" #include "llvm/IR/CallSite.h" @@ -1049,32 +1048,7 @@ bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock, // backwards through predecessors if needed. BasicBlock *LoadBB = LI->getParent(); BasicBlock *TmpBB = LoadBB; - bool IsSafeToSpeculativelyExecute = isSafeToSpeculativelyExecute(LI); - // Check that there is no implicit control flow instructions above our load in - // its block. If there is an instruction that doesn't always pass the - // execution to the following instruction, then moving through it may become - // invalid. For example: - // - // int arr[LEN]; - // int index = ???; - // ... - // guard(0 <= index && index < LEN); - // use(arr[index]); - // - // It is illegal to move the array access to any point above the guard, - // because if the index is out of bounds we should deoptimize rather than - // access the array. - // Check that there is no guard in this block above our intruction. - if (!IsSafeToSpeculativelyExecute) { - auto It = FirstImplicitControlFlowInsts.find(TmpBB); - if (It != FirstImplicitControlFlowInsts.end()) { - assert(It->second->getParent() == TmpBB && - "Implicit control flow map broken?"); - if (OI->dominates(It->second, LI)) - return false; - } - } while (TmpBB->getSinglePredecessor()) { TmpBB = TmpBB->getSinglePredecessor(); if (TmpBB == LoadBB) // Infinite (unreachable) loop. @@ -1089,11 +1063,6 @@ bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock, // which it was not previously executed. if (TmpBB->getTerminator()->getNumSuccessors() != 1) return false; - - // Check that there is no implicit control flow in a block above. - if (!IsSafeToSpeculativelyExecute && - FirstImplicitControlFlowInsts.count(TmpBB)) - return false; } assert(TmpBB); @@ -2014,8 +1983,6 @@ bool GVN::runImpl(Function &F, AssumptionCache &RunAC, DominatorTree &RunDT, TLI = &RunTLI; VN.setAliasAnalysis(&RunAA); MD = RunMD; - OrderedInstructions OrderedInstrs(DT); - OI = &OrderedInstrs; VN.setMemDep(MD); ORE = RunORE; @@ -2105,9 +2072,6 @@ bool GVN::processBlock(BasicBlock *BB) { DEBUG(verifyRemoved(*I)); (*I)->eraseFromParent(); } - - if (!InstrsToErase.empty()) - OI->invalidateBlock(BB); InstrsToErase.clear(); if (AtStart) @@ -2303,7 +2267,6 @@ bool GVN::performScalarPRE(Instruction *CurInst) { MD->removeInstruction(CurInst); DEBUG(verifyRemoved(CurInst)); CurInst->eraseFromParent(); - OI->invalidateBlock(CurrentBlock); ++NumGVNInstr; return true; @@ -2370,7 +2333,6 @@ bool GVN::iterateOnFunction(Function &F) { // RPOT walks the graph in its constructor and will not be invalidated during // processBlock. ReversePostOrderTraversal RPOT(&F); - fillImplicitControlFlowInfo(RPOT); for (BasicBlock *BB : RPOT) Changed |= processBlock(BB); @@ -2382,45 +2344,6 @@ void GVN::cleanupGlobalSets() { LeaderTable.clear(); BlockRPONumber.clear(); TableAllocator.Reset(); - FirstImplicitControlFlowInsts.clear(); -} - -void -GVN::fillImplicitControlFlowInfo(ReversePostOrderTraversal &RPOT) { - auto MayNotTransferExecutionToSuccessor = [&](const Instruction *I) { - // If a block's instruction doesn't always pass the control to its successor - // instruction, mark the block as having implicit control flow. We use them - // to avoid wrong assumptions of sort "if A is executed and B post-dominates - // A, then B is also executed". This is not true is there is an implicit - // control flow instruction (e.g. a guard) between them. - // - // TODO: Currently, isGuaranteedToTransferExecutionToSuccessor returns false - // for volatile stores and loads because they can trap. The discussion on - // whether or not it is correct is still ongoing. We might want to get rid - // of this logic in the future. Anyways, trapping instructions shouldn't - // introduce implicit control flow, so we explicitly allow them here. This - // must be removed once isGuaranteedToTransferExecutionToSuccessor is fixed. - if (isGuaranteedToTransferExecutionToSuccessor(I)) - return false; - if (isa(I)) { - assert(cast(I)->isVolatile() && - "Non-volatile load should transfer execution to successor!"); - return false; - } - if (isa(I)) { - assert(cast(I)->isVolatile() && - "Non-volatile store should transfer execution to successor!"); - return false; - } - return true; - }; - - for (BasicBlock *BB : RPOT) - for (auto &I : *BB) - if (MayNotTransferExecutionToSuccessor(&I)) { - FirstImplicitControlFlowInsts[BB] = &I; - break; - } } /// Verify that the specified instruction does not occur in our diff --git a/llvm/test/Transforms/GVN/PRE/pre-load.ll b/llvm/test/Transforms/GVN/PRE/pre-load.ll index 9123cf1..ffff2b7 100644 --- a/llvm/test/Transforms/GVN/PRE/pre-load.ll +++ b/llvm/test/Transforms/GVN/PRE/pre-load.ll @@ -430,31 +430,3 @@ cleanup2: call void @g(i32 %NOTPRE) cleanupret from %c2 unwind to caller } - -; Don't PRE load across calls. - -define i32 @test13(i32* noalias nocapture readonly %x, i32* noalias nocapture %r, i32 %a) { -; CHECK-LABEL: @test13( -; CHECK: entry: -; CHECK-NEXT: icmp eq -; CHECK-NEXT: br i1 -entry: - %tobool = icmp eq i32 %a, 0 - br i1 %tobool, label %if.end, label %if.then - -; CHECK: if.then: -; CHECK-NEXT: load i32 -; CHECK-NEXT: store i32 -if.then: - %uu = load i32, i32* %x, align 4 - store i32 %uu, i32* %r, align 4 - br label %if.end - -; CHECK: if.end: -; CHECK-NEXT: call void @f() -; CHECK-NEXT: load i32 -if.end: - call void @f() - %vv = load i32, i32* %x, align 4 - ret i32 %vv -} -- 2.7.4