[llvm] r315974 - Revert 315440 on behalf of mkazantsev
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 16 23:21:07 PDT 2017
Author: reames
Date: Mon Oct 16 23:21:07 2017
New Revision: 315974
URL: http://llvm.org/viewvc/llvm-project?rev=315974&view=rev
Log:
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.
Modified:
llvm/trunk/include/llvm/Transforms/Scalar/GVN.h
llvm/trunk/lib/Transforms/Scalar/GVN.cpp
llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll
Modified: llvm/trunk/include/llvm/Transforms/Scalar/GVN.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Scalar/GVN.h?rev=315974&r1=315973&r2=315974&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Transforms/Scalar/GVN.h (original)
+++ llvm/trunk/include/llvm/Transforms/Scalar/GVN.h Mon Oct 16 23:21:07 2017
@@ -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 <cstdint>
#include <utility>
#include <vector>
@@ -158,11 +156,7 @@ private:
AssumptionCache *AC;
SetVector<BasicBlock *> DeadBlocks;
OptimizationRemarkEmitter *ORE;
- // Maps a block to the topmost instruction with implicit control flow in it.
- DenseMap<const BasicBlock *, const Instruction *>
- 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<Function *> &RPOT);
void verifyRemoved(const Instruction *I) const;
bool splitCriticalEdges();
BasicBlock *splitCriticalEdges(BasicBlock *Pred, BasicBlock *Succ);
Modified: llvm/trunk/lib/Transforms/Scalar/GVN.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=315974&r1=315973&r2=315974&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Mon Oct 16 23:21:07 2017
@@ -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, A
// 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, A
// 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, Assumptio
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 *
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<Function *> 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<Function *> &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<LoadInst>(I)) {
- assert(cast<LoadInst>(I)->isVolatile() &&
- "Non-volatile load should transfer execution to successor!");
- return false;
- }
- if (isa<StoreInst>(I)) {
- assert(cast<StoreInst>(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
Modified: llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll?rev=315974&r1=315973&r2=315974&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll (original)
+++ llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll Mon Oct 16 23:21:07 2017
@@ -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
-}
More information about the llvm-commits
mailing list