Skip to content

Commit

Permalink
Revert "[GVN] Improve PRE on load instructions"
Browse files Browse the repository at this point in the history 
This reverts commit d681182.

It caused sanitized bootstrap failure.
  • Loading branch information
@weiguozhi
weiguozhi committed May 17, 2023
1 parent 6012cad commit a3fbe5f
Show file tree
Hide file tree
Showing 10 changed files with 84 additions and 530 deletions.
9 changes: 1 addition & 8 deletions llvm/include/llvm/Transforms/Scalar/GVN.h
View file
Expand Up @@ -329,11 +329,6 @@ class GVNPass : public PassInfoMixin<GVNPass> {
AvailValInBlkVect &ValuesPerBlock,
UnavailBlkVect &UnavailableBlocks);

/// Given a critical edge from Pred to LoadBB, find a load instruction
/// which is identical to Load from another successor of Pred.
LoadInst *findLoadToHoistIntoPred(BasicBlock *Pred, BasicBlock *LoadBB,
LoadInst *Load);

bool PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
UnavailBlkVect &UnavailableBlocks);

Expand All @@ -347,8 +342,7 @@ class GVNPass : public PassInfoMixin<GVNPass> {
/// AvailableLoads (connected by Phis if needed).
void eliminatePartiallyRedundantLoad(
LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
MapVector<BasicBlock *, Value *> &AvailableLoads,
MapVector<BasicBlock *, LoadInst *> *CriticalEdgePredAndLoad);
MapVector<BasicBlock *, Value *> &AvailableLoads);

// Other helper routines
bool processInstruction(Instruction *I);
Expand All @@ -361,7 +355,6 @@ class GVNPass : public PassInfoMixin<GVNPass> {
BasicBlock *Curr, unsigned int ValNo);
Value *findLeader(const BasicBlock *BB, uint32_t num);
void cleanupGlobalSets();
void removeInstruction(Instruction *I);
void verifyRemoved(const Instruction *I) const;
bool splitCriticalEdges();
BasicBlock *splitCriticalEdges(BasicBlock *Pred, BasicBlock *Succ);
Expand Down
165 changes: 26 additions & 139 deletions llvm/lib/Transforms/Scalar/GVN.cpp
View file
Expand Up @@ -94,8 +94,6 @@ STATISTIC(NumGVNSimpl, "Number of instructions simplified");
STATISTIC(NumGVNEqProp, "Number of equalities propagated");
STATISTIC(NumPRELoad, "Number of loads PRE'd");
STATISTIC(NumPRELoopLoad, "Number of loop loads PRE'd");
STATISTIC(NumPRELoadMoved2CEPred,
"Number of loads moved to predecessor of a critical edge in PRE");

STATISTIC(IsValueFullyAvailableInBlockNumSpeculationsMax,
"Number of blocks speculated as available in "
Expand Down Expand Up @@ -129,11 +127,6 @@ static cl::opt<uint32_t> MaxNumVisitedInsts(
cl::desc("Max number of visited instructions when trying to find "
"dominating value of select dependency (default = 100)"));

static cl::opt<uint32_t> MaxNumInsnsPerBlock(
"gvn-max-num-insns", cl::Hidden, cl::init(100),
cl::desc("Max number of instructions to scan in each basic block in GVN "
"(default = 100)"));

struct llvm::GVNPass::Expression {
uint32_t opcode;
bool commutative = false;
Expand Down Expand Up @@ -937,21 +930,6 @@ static bool IsValueFullyAvailableInBlock(
return !UnavailableBB;
}

/// If the specified (BB, OldValue) exists in ValuesPerBlock, replace its value
/// with NewValue, otherwise we don't change it.
static void replaceValuesPerBlockEntry(
SmallVectorImpl<AvailableValueInBlock> &ValuesPerBlock, BasicBlock *BB,
Value *OldValue, Value *NewValue) {
for (AvailableValueInBlock &V : ValuesPerBlock) {
if (V.BB == BB) {
if ((V.AV.isSimpleValue() && V.AV.getSimpleValue() == OldValue) ||
(V.AV.isCoercedLoadValue() && V.AV.getCoercedLoadValue() == OldValue))
V = AvailableValueInBlock::get(BB, NewValue);
return;
}
}
}

/// Given a set of loads specified by ValuesPerBlock,
/// construct SSA form, allowing us to eliminate Load. This returns the value
/// that should be used at Load's definition site.
Expand Down Expand Up @@ -1345,67 +1323,9 @@ void GVNPass::AnalyzeLoadAvailability(LoadInst *Load, LoadDepVect &Deps,
"post condition violation");
}

/// Given the following code, v1 is partially available on some edges, but not
/// available on the edge from PredBB. This function tries to find if there is
/// another identical load in the other successor of PredBB.
///
/// v0 = load %addr
/// br %LoadBB
///
/// LoadBB:
/// v1 = load %addr
/// ...
///
/// PredBB:
/// ...
/// br %cond, label %LoadBB, label %SuccBB
///
/// SuccBB:
/// v2 = load %addr
/// ...
///
LoadInst *GVNPass::findLoadToHoistIntoPred(BasicBlock *Pred, BasicBlock *LoadBB,
LoadInst *Load) {
// For simplicity we handle a Pred has 2 successors only.
auto *Term = Pred->getTerminator();
if (Term->getNumSuccessors() != 2 || Term->isExceptionalTerminator())
return nullptr;
auto *SuccBB = Term->getSuccessor(0);
if (SuccBB == LoadBB)
SuccBB = Term->getSuccessor(1);
if (!SuccBB->getSinglePredecessor())
return nullptr;

unsigned int NumInsts = MaxNumInsnsPerBlock;
for (Instruction &Inst : *SuccBB) {
if (Inst.isDebugOrPseudoInst())
continue;
if (--NumInsts == 0)
return nullptr;

if (!Inst.isIdenticalTo(Load))
continue;

MemDepResult Dep = MD->getDependency(&Inst);
// If an identical load doesn't depends on any local instructions, it can
// be safely moved to PredBB.
// Also check for the implicit control flow instructions. See the comments
// in PerformLoadPRE for details.
if (Dep.isNonLocal() && !ICF->isDominatedByICFIFromSameBlock(&Inst))
return cast<LoadInst>(&Inst);

// Otherwise there is something in the same BB clobbers the memory, we can't
// move this and later load to PredBB.
return nullptr;
}

return nullptr;
}

void GVNPass::eliminatePartiallyRedundantLoad(
LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
MapVector<BasicBlock *, Value *> &AvailableLoads,
MapVector<BasicBlock *, LoadInst *> *CriticalEdgePredAndLoad) {
MapVector<BasicBlock *, Value *> &AvailableLoads) {
for (const auto &AvailableLoad : AvailableLoads) {
BasicBlock *UnavailableBlock = AvailableLoad.first;
Value *LoadPtr = AvailableLoad.second;
Expand Down Expand Up @@ -1459,24 +1379,6 @@ void GVNPass::eliminatePartiallyRedundantLoad(
AvailableValueInBlock::get(UnavailableBlock, NewLoad));
MD->invalidateCachedPointerInfo(LoadPtr);
LLVM_DEBUG(dbgs() << "GVN INSERTED " << *NewLoad << '\n');

// For PredBB in CriticalEdgePredAndLoad we need to replace the uses of old
// load instruction with the new created load instruction.
if (CriticalEdgePredAndLoad) {
auto I = CriticalEdgePredAndLoad->find(UnavailableBlock);
if (I != CriticalEdgePredAndLoad->end()) {
++NumPRELoadMoved2CEPred;
ICF->insertInstructionTo(NewLoad, UnavailableBlock);
LoadInst *OldLoad = I->second;
combineMetadataForCSE(NewLoad, OldLoad, false);
OldLoad->replaceAllUsesWith(NewLoad);
replaceValuesPerBlockEntry(ValuesPerBlock, OldLoad->getParent(),
OldLoad, NewLoad);
if (uint32_t ValNo = VN.lookup(OldLoad, false))
removeFromLeaderTable(ValNo, OldLoad, OldLoad->getParent());
removeInstruction(OldLoad);
}
}
}

// Perform PHI construction.
Expand Down Expand Up @@ -1564,12 +1466,7 @@ bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
for (BasicBlock *UnavailableBB : UnavailableBlocks)
FullyAvailableBlocks[UnavailableBB] = AvailabilityState::Unavailable;

// The edge from Pred to LoadBB is a critical edge will be splitted.
SmallVector<BasicBlock *, 4> CriticalEdgePredSplit;
// The edge from Pred to LoadBB is a critical edge, another successor of Pred
// contains a load can be moved to Pred. This data structure maps the Pred to
// the movable load.
MapVector<BasicBlock *, LoadInst *> CriticalEdgePredAndLoad;
SmallVector<BasicBlock *, 4> CriticalEdgePred;
for (BasicBlock *Pred : predecessors(LoadBB)) {
// If any predecessor block is an EH pad that does not allow non-PHI
// instructions before the terminator, we can't PRE the load.
Expand Down Expand Up @@ -1609,59 +1506,47 @@ bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
return false;
}

if (LoadInst *LI = findLoadToHoistIntoPred(Pred, LoadBB, Load))
CriticalEdgePredAndLoad[Pred] = LI;
else
CriticalEdgePredSplit.push_back(Pred);
CriticalEdgePred.push_back(Pred);
} else {
// Only add the predecessors that will not be split for now.
PredLoads[Pred] = nullptr;
}
}

// Decide whether PRE is profitable for this load.
unsigned NumInsertPreds = PredLoads.size() + CriticalEdgePredSplit.size();
unsigned NumUnavailablePreds = NumInsertPreds +
CriticalEdgePredAndLoad.size();
unsigned NumUnavailablePreds = PredLoads.size() + CriticalEdgePred.size();
assert(NumUnavailablePreds != 0 &&
"Fully available value should already be eliminated!");
(void)NumUnavailablePreds;

// If we need to insert new load in multiple predecessors, reject it.
// If this load is unavailable in multiple predecessors, reject it.
// FIXME: If we could restructure the CFG, we could make a common pred with
// all the preds that don't have an available Load and insert a new load into
// that one block.
if (NumInsertPreds > 1)
if (NumUnavailablePreds != 1)
return false;

// Now we know where we will insert load. We must ensure that it is safe
// to speculatively execute the load at that points.
if (MustEnsureSafetyOfSpeculativeExecution) {
if (CriticalEdgePredSplit.size())
if (CriticalEdgePred.size())
if (!isSafeToSpeculativelyExecute(Load, LoadBB->getFirstNonPHI(), AC, DT))
return false;
for (auto &PL : PredLoads)
if (!isSafeToSpeculativelyExecute(Load, PL.first->getTerminator(), AC,
DT))
return false;
for (auto &CEP : CriticalEdgePredAndLoad)
if (!isSafeToSpeculativelyExecute(Load, CEP.first->getTerminator(), AC,
DT))
return false;
}

// Split critical edges, and update the unavailable predecessors accordingly.
for (BasicBlock *OrigPred : CriticalEdgePredSplit) {
for (BasicBlock *OrigPred : CriticalEdgePred) {
BasicBlock *NewPred = splitCriticalEdges(OrigPred, LoadBB);
assert(!PredLoads.count(OrigPred) && "Split edges shouldn't be in map!");
PredLoads[NewPred] = nullptr;
LLVM_DEBUG(dbgs() << "Split critical edge " << OrigPred->getName() << "->"
<< LoadBB->getName() << '\n');
}

for (auto &CEP : CriticalEdgePredAndLoad)
PredLoads[CEP.first] = nullptr;

// Check if the load can safely be moved to all the unavailable predecessors.
bool CanDoPRE = true;
const DataLayout &DL = Load->getModule()->getDataLayout();
Expand Down Expand Up @@ -1718,7 +1603,7 @@ bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
}
// HINT: Don't revert the edge-splitting as following transformation may
// also need to split these critical edges.
return !CriticalEdgePredSplit.empty();
return !CriticalEdgePred.empty();
}

// Okay, we can eliminate this load by inserting a reload in the predecessor
Expand All @@ -1743,8 +1628,7 @@ bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
VN.lookupOrAdd(I);
}

eliminatePartiallyRedundantLoad(Load, ValuesPerBlock, PredLoads,
&CriticalEdgePredAndLoad);
eliminatePartiallyRedundantLoad(Load, ValuesPerBlock, PredLoads);
++NumPRELoad;
return true;
}
Expand Down Expand Up @@ -1823,8 +1707,7 @@ bool GVNPass::performLoopLoadPRE(LoadInst *Load,
AvailableLoads[Preheader] = LoadPtr;

LLVM_DEBUG(dbgs() << "GVN REMOVING PRE LOOP LOAD: " << *Load << '\n');
eliminatePartiallyRedundantLoad(Load, ValuesPerBlock, AvailableLoads,
/*CriticalEdgePredAndLoad*/ nullptr);
eliminatePartiallyRedundantLoad(Load, ValuesPerBlock, AvailableLoads);
++NumPRELoopLoad;
return true;
}
Expand Down Expand Up @@ -2800,7 +2683,12 @@ bool GVNPass::processBlock(BasicBlock *BB) {
LLVM_DEBUG(dbgs() << "GVN removed: " << *I << '\n');
salvageKnowledge(I, AC);
salvageDebugInfo(*I);
removeInstruction(I);
if (MD) MD->removeInstruction(I);
if (MSSAU)
MSSAU->removeMemoryAccess(I);
LLVM_DEBUG(verifyRemoved(I));
ICF->removeInstruction(I);
I->eraseFromParent();
}
InstrsToErase.clear();

Expand Down Expand Up @@ -3020,7 +2908,15 @@ bool GVNPass::performScalarPRE(Instruction *CurInst) {
removeFromLeaderTable(ValNo, CurInst, CurrentBlock);

LLVM_DEBUG(dbgs() << "GVN PRE removed: " << *CurInst << '\n');
removeInstruction(CurInst);
if (MD)
MD->removeInstruction(CurInst);
if (MSSAU)
MSSAU->removeMemoryAccess(CurInst);
LLVM_DEBUG(verifyRemoved(CurInst));
// FIXME: Intended to be markInstructionForDeletion(CurInst), but it causes
// some assertion failures.
ICF->removeInstruction(CurInst);
CurInst->eraseFromParent();
++NumGVNInstr;

return true;
Expand Down Expand Up @@ -3116,15 +3012,6 @@ void GVNPass::cleanupGlobalSets() {
InvalidBlockRPONumbers = true;
}

void GVNPass::removeInstruction(Instruction *I) {
if (MD) MD->removeInstruction(I);
if (MSSAU)
MSSAU->removeMemoryAccess(I);
LLVM_DEBUG(verifyRemoved(I));
ICF->removeInstruction(I);
I->eraseFromParent();
}

/// Verify that the specified instruction does not occur in our
/// internal data structures.
void GVNPass::verifyRemoved(const Instruction *Inst) const {
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/Transforms/GVN/PRE/2011-06-01-NonLocalMemdepMiscompile.ll
View file
Expand Up @@ -57,7 +57,7 @@ bb15:
; CHECK-NEXT: br label %bb15

; CHECK-LABEL: bb15:
; CHECK: %tmp17 = phi i8 [ %tmp12.pre3, %bb15split ], [ %tmp17.pre, %bb1.bb15_crit_edge ]
; CHECK: %tmp17 = phi i8 [ %tmp8, %bb15split ], [ %tmp17.pre, %bb1.bb15_crit_edge ]

bb19: ; preds = %bb15
ret i1 %tmp18
Expand Down
13 changes: 7 additions & 6 deletions llvm/test/Transforms/GVN/PRE/2017-06-28-pre-load-dbgloc.ll
View file
@@ -1,6 +1,6 @@
; This test checks if debug loc is propagated to load/store created by GVN/Instcombine.
; RUN: opt < %s -passes=gvn -S | FileCheck %s --check-prefixes=ALL
; RUN: opt < %s -passes=gvn,instcombine -S | FileCheck %s --check-prefixes=ALL
; RUN: opt < %s -passes=gvn -S | FileCheck %s --check-prefixes=ALL,GVN
; RUN: opt < %s -passes=gvn,instcombine -S | FileCheck %s --check-prefixes=ALL,INSTCOMBINE

; struct node {
; int *v;
Expand Down Expand Up @@ -35,9 +35,10 @@ define i32 @test(ptr readonly %desc) local_unnamed_addr #0 !dbg !4 {
entry:
%tobool = icmp eq ptr %desc, null
br i1 %tobool, label %cond.end, label %cond.false, !dbg !9
; ALL: %.pre = load ptr, ptr %desc, align 8, !dbg [[LOC_16_13:![0-9]+]]
; ALL: br i1 %tobool, label %cond.end, label %cond.false, !dbg [[LOC_15_6:![0-9]+]]
; ALL: cond.false:
; ALL: br i1 %tobool, label %entry.cond.end_crit_edge, label %cond.false, !dbg [[LOC_15_6:![0-9]+]]
; ALL: entry.cond.end_crit_edge:
; GVN: %.pre = load ptr, ptr null, align 8, !dbg [[LOC_16_13:![0-9]+]]
; INSTCOMBINE:store ptr poison, ptr null, align 4294967296, !dbg [[LOC_16_13:![0-9]+]]

cond.false:
%0 = load ptr, ptr %desc, align 8, !dbg !11
Expand Down Expand Up @@ -71,5 +72,5 @@ declare i32 @bar(ptr, ptr) local_unnamed_addr #1
!11 = !DILocation(line: 15, column: 34, scope: !4)

;ALL: [[SCOPE:![0-9]+]] = distinct !DISubprogram(name: "test",{{.*}}
;ALL: [[LOC_16_13]] = !DILocation(line: 16, column: 13, scope: [[SCOPE]])
;ALL: [[LOC_15_6]] = !DILocation(line: 15, column: 6, scope: [[SCOPE]])
;ALL: [[LOC_16_13]] = !DILocation(line: 16, column: 13, scope: [[SCOPE]])
11 changes: 6 additions & 5 deletions llvm/test/Transforms/GVN/PRE/2018-06-08-pre-load-dbgloc-no-null-opt.ll
View file
Expand Up @@ -35,17 +35,18 @@ define i32 @test_no_null_opt(ptr readonly %desc) local_unnamed_addr #0 !dbg !4 {
entry:
%tobool = icmp eq ptr %desc, null
br i1 %tobool, label %cond.end, label %cond.false, !dbg !9
; ALL: %.pre = load ptr, ptr %desc, align 8, !dbg [[LOC_16_13:![0-9]+]]
; ALL: br i1 %tobool, label %cond.end, label %cond.false, !dbg [[LOC_15_6:![0-9]+]]
; ALL: cond.false:
; ALL: br i1 %tobool, label %entry.cond.end_crit_edge, label %cond.false, !dbg [[LOC_15_6:![0-9]+]]
; ALL: entry.cond.end_crit_edge:
; ALL: load ptr, ptr null, align {{[0-9]+}}, !dbg [[LOC_16_13:![0-9]+]]

cond.false:
%0 = load ptr, ptr %desc, align 8, !dbg !11
%1 = load ptr, ptr %0, align 8
br label %cond.end, !dbg !9

cond.end:
; ALL: phi ptr [ %0, %cond.false ], [ null, %entry ]
; ALL: phi ptr [ %0, %cond.false ], [ %.pre, %entry.cond.end_crit_edge ]
; ALL: phi ptr [ %1, %cond.false ], [ null, %entry.cond.end_crit_edge ]

%2 = phi ptr [ %1, %cond.false ], [ null, %entry ], !dbg !9
%3 = load ptr, ptr %desc, align 8, !dbg !10
Expand Down Expand Up @@ -74,5 +75,5 @@ declare i32 @bar(ptr, ptr) local_unnamed_addr #1
!11 = !DILocation(line: 15, column: 34, scope: !4)

;ALL: [[SCOPE:![0-9]+]] = distinct !DISubprogram(name: "test_no_null_opt",{{.*}}
;ALL: [[LOC_16_13]] = !DILocation(line: 16, column: 13, scope: [[SCOPE]])
;ALL: [[LOC_15_6]] = !DILocation(line: 15, column: 6, scope: [[SCOPE]])
;ALL: [[LOC_16_13]] = !DILocation(line: 16, column: 13, scope: [[SCOPE]])

0 comments on commit a3fbe5f

Please sign in to comment.