Skip to content

Commit

Permalink
Revert "[SimplifyCFG] Transform for redirecting phis between unmergea…
Browse files Browse the repository at this point in the history 
…ble BB and SuccBB (#67275)"

This reverts commit fc86d03.

This change breaks LLVM buildbot clang-aarch64-sve-vls-2stage
https://lab.llvm.org/buildbot/#/builders/176/builds/5474
I am going to revert this patch as the bot has been failing for more than a day without a fix.
  • Loading branch information
@omjavaid
omjavaid committed Sep 26, 2023
1 parent 5c6eefb commit 431969e
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 789 deletions.
166 changes: 38 additions & 128 deletions llvm/lib/Transforms/Utils/Local.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -847,17 +847,17 @@ static bool CanMergeValues(Value *First, Value *Second) {
/// branch to Succ, into Succ.
///
/// Assumption: Succ is the single successor for BB.
static bool
CanPropagatePredecessorsForPHIs(BasicBlock *BB, BasicBlock *Succ,
const SmallPtrSetImpl<BasicBlock *> &BBPreds) {
static bool CanPropagatePredecessorsForPHIs(BasicBlock *BB, BasicBlock *Succ) {
assert(*succ_begin(BB) == Succ && "Succ is not successor of BB!");

LLVM_DEBUG(dbgs() << "Looking to fold " << BB->getName() << " into "
<< Succ->getName() << "\n");
// Shortcut, if there is only a single predecessor it must be BB and merging
// is always safe
if (Succ->getSinglePredecessor())
return true;
if (Succ->getSinglePredecessor()) return true;

// Make a list of the predecessors of BB
SmallPtrSet<BasicBlock*, 16> BBPreds(pred_begin(BB), pred_end(BB));

// Look at all the phi nodes in Succ, to see if they present a conflict when
// merging these blocks
Expand Down Expand Up @@ -997,47 +997,16 @@ static void replaceUndefValuesInPhi(PHINode *PN,
}
}

// Only when they shares a single common predecessor, return true.
// Only handles cases when BB can't be merged while its predecessors can be
// redirected.
static bool
CanRedirectPredsOfEmptyBBToSucc(BasicBlock *BB, BasicBlock *Succ,
const SmallPtrSetImpl<BasicBlock *> &BBPreds,
const SmallPtrSetImpl<BasicBlock *> &SuccPreds,
BasicBlock *&CommonPred) {

// There must be phis in BB, otherwise BB will be merged into Succ directly
if (BB->phis().empty() || Succ->phis().empty())
return false;

// BB must have predecessors not shared that can be redirected to Succ
if (!BB->hasNPredecessorsOrMore(2))
return false;

// Get single common predecessors of both BB and Succ
for (BasicBlock *SuccPred : SuccPreds) {
if (BBPreds.count(SuccPred)) {
if (CommonPred)
return false;
CommonPred = SuccPred;
}
}

return true;
}

/// Replace a value flowing from a block to a phi with
/// potentially multiple instances of that value flowing from the
/// block's predecessors to the phi.
///
/// \param BB The block with the value flowing into the phi.
/// \param BBPreds The predecessors of BB.
/// \param PN The phi that we are updating.
/// \param CommonPred The common predecessor of BB and PN's BasicBlock
static void redirectValuesFromPredecessorsToPhi(BasicBlock *BB,
const PredBlockVector &BBPreds,
PHINode *PN,
BasicBlock *CommonPred) {
PHINode *PN) {
Value *OldVal = PN->removeIncomingValue(BB, false);
assert(OldVal && "No entry in PHI for Pred BB!");

Expand Down Expand Up @@ -1065,39 +1034,26 @@ static void redirectValuesFromPredecessorsToPhi(BasicBlock *BB,
// will trigger asserts if we try to clean it up now, without also
// simplifying the corresponding conditional branch).
BasicBlock *PredBB = OldValPN->getIncomingBlock(i);

if (PredBB == CommonPred)
continue;

Value *PredVal = OldValPN->getIncomingValue(i);
Value *Selected =
selectIncomingValueForBlock(PredVal, PredBB, IncomingValues);
Value *Selected = selectIncomingValueForBlock(PredVal, PredBB,
IncomingValues);

// And add a new incoming value for this predecessor for the
// newly retargeted branch.
PN->addIncoming(Selected, PredBB);
}
if (CommonPred)
PN->addIncoming(OldValPN->getIncomingValueForBlock(CommonPred), BB);

} else {
for (unsigned i = 0, e = BBPreds.size(); i != e; ++i) {
// Update existing incoming values in PN for this
// predecessor of BB.
BasicBlock *PredBB = BBPreds[i];

if (PredBB == CommonPred)
continue;

Value *Selected =
selectIncomingValueForBlock(OldVal, PredBB, IncomingValues);
Value *Selected = selectIncomingValueForBlock(OldVal, PredBB,
IncomingValues);

// And add a new incoming value for this predecessor for the
// newly retargeted branch.
PN->addIncoming(Selected, PredBB);
}
if (CommonPred)
PN->addIncoming(OldVal, BB);
}

replaceUndefValuesInPhi(PN, IncomingValues);
Expand All @@ -1108,30 +1064,13 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
assert(BB != &BB->getParent()->getEntryBlock() &&
"TryToSimplifyUncondBranchFromEmptyBlock called on entry block!");

// We can't simplify infinite loops.
// We can't eliminate infinite loops.
BasicBlock *Succ = cast<BranchInst>(BB->getTerminator())->getSuccessor(0);
if (BB == Succ)
return false;

SmallPtrSet<BasicBlock *, 16> BBPreds(pred_begin(BB), pred_end(BB));
SmallPtrSet<BasicBlock *, 16> SuccPreds(pred_begin(Succ), pred_end(Succ));
if (BB == Succ) return false;

// The single common predecessor of BB and Succ when BB cannot be killed
BasicBlock *CommonPred = nullptr;

bool BBKillable = CanPropagatePredecessorsForPHIs(BB, Succ, BBPreds);

// Even if we can not fold bB into Succ, we may be able to redirect the
// predecessors of BB to Succ.
bool BBPhisMergeable =
BBKillable ||
CanRedirectPredsOfEmptyBBToSucc(BB, Succ, BBPreds, SuccPreds, CommonPred);

if (!BBKillable && !BBPhisMergeable)
return false;

// Check to see if merging these blocks/phis would cause conflicts for any of
// the phi nodes in BB or Succ. If not, we can safely merge.
// Check to see if merging these blocks would cause conflicts for any of the
// phi nodes in BB or Succ. If not, we can safely merge.
if (!CanPropagatePredecessorsForPHIs(BB, Succ)) return false;

// Check for cases where Succ has multiple predecessors and a PHI node in BB
// has uses which will not disappear when the PHI nodes are merged. It is
Expand Down Expand Up @@ -1160,11 +1099,6 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
}
}

if (BBPhisMergeable && CommonPred)
LLVM_DEBUG(dbgs() << "Found Common Predecessor between: " << BB->getName()
<< " and " << Succ->getName() << " : "
<< CommonPred->getName() << "\n");

// 'BB' and 'BB->Pred' are loop latches, bail out to presrve inner loop
// metadata.
//
Expand Down Expand Up @@ -1237,37 +1171,25 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
if (PredTI->hasMetadata(LLVMContext::MD_loop))
return false;

if (BBKillable)
LLVM_DEBUG(dbgs() << "Killing Trivial BB: \n" << *BB);
else if (BBPhisMergeable)
LLVM_DEBUG(dbgs() << "Merge Phis in Trivial BB: \n" << *BB);
LLVM_DEBUG(dbgs() << "Killing Trivial BB: \n" << *BB);

SmallVector<DominatorTree::UpdateType, 32> Updates;

if (DTU) {
// To avoid processing the same predecessor more than once.
SmallPtrSet<BasicBlock *, 8> SeenPreds;
// All predecessors of BB (except the common predecessor) will be moved to
// Succ.
// All predecessors of BB will be moved to Succ.
SmallPtrSet<BasicBlock *, 8> PredsOfSucc(pred_begin(Succ), pred_end(Succ));
Updates.reserve(Updates.size() + 2 * pred_size(BB) + 1);

for (auto *PredOfBB : predecessors(BB)) {
// Do not modify those common predecessors of BB and Succ
if (!SuccPreds.contains(PredOfBB))
for (auto *PredOfBB : predecessors(BB))
// This predecessor of BB may already have Succ as a successor.
if (!PredsOfSucc.contains(PredOfBB))
if (SeenPreds.insert(PredOfBB).second)
Updates.push_back({DominatorTree::Insert, PredOfBB, Succ});
}

SeenPreds.clear();

for (auto *PredOfBB : predecessors(BB))
// When BB cannot be killed, do not remove the edge between BB and
// CommonPred.
if (SeenPreds.insert(PredOfBB).second && PredOfBB != CommonPred)
if (SeenPreds.insert(PredOfBB).second)
Updates.push_back({DominatorTree::Delete, PredOfBB, BB});

if (BBKillable)
Updates.push_back({DominatorTree::Delete, BB, Succ});
Updates.push_back({DominatorTree::Delete, BB, Succ});
}

if (isa<PHINode>(Succ->begin())) {
Expand All @@ -1279,19 +1201,21 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
// Loop over all of the PHI nodes in the successor of BB.
for (BasicBlock::iterator I = Succ->begin(); isa<PHINode>(I); ++I) {
PHINode *PN = cast<PHINode>(I);
redirectValuesFromPredecessorsToPhi(BB, BBPreds, PN, CommonPred);

redirectValuesFromPredecessorsToPhi(BB, BBPreds, PN);
}
}

if (Succ->getSinglePredecessor()) {
// BB is the only predecessor of Succ, so Succ will end up with exactly
// the same predecessors BB had.

// Copy over any phi, debug or lifetime instruction.
BB->getTerminator()->eraseFromParent();
Succ->splice(Succ->getFirstNonPHI()->getIterator(), BB);
} else {
while (PHINode *PN = dyn_cast<PHINode>(&BB->front())) {
// We explicitly check for such uses for merging phis.
// We explicitly check for such uses in CanPropagatePredecessorsForPHIs.
assert(PN->use_empty() && "There shouldn't be any uses here!");
PN->eraseFromParent();
}
Expand All @@ -1304,35 +1228,21 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
for (BasicBlock *Pred : predecessors(BB))
Pred->getTerminator()->setMetadata(LLVMContext::MD_loop, LoopMD);

if (BBKillable) {
// Everything that jumped to BB now goes to Succ.
BB->replaceAllUsesWith(Succ);

if (!Succ->hasName())
Succ->takeName(BB);

// Clear the successor list of BB to match updates applying to DTU later.
if (BB->getTerminator())
BB->back().eraseFromParent();

new UnreachableInst(BB->getContext(), BB);
assert(succ_empty(BB) && "The successor list of BB isn't empty before "
"applying corresponding DTU updates.");
} else if (BBPhisMergeable) {
// Everything except CommonPred that jumped to BB now goes to Succ.
BB->replaceUsesWithIf(Succ, [BBPreds, CommonPred](Use &U) -> bool {
if (Instruction *UseInst = dyn_cast<Instruction>(U.getUser()))
return UseInst->getParent() != CommonPred &&
BBPreds.contains(UseInst->getParent());
return false;
});
}
// Everything that jumped to BB now goes to Succ.
BB->replaceAllUsesWith(Succ);
if (!Succ->hasName()) Succ->takeName(BB);

// Clear the successor list of BB to match updates applying to DTU later.
if (BB->getTerminator())
BB->back().eraseFromParent();
new UnreachableInst(BB->getContext(), BB);
assert(succ_empty(BB) && "The successor list of BB isn't empty before "
"applying corresponding DTU updates.");

if (DTU)
DTU->applyUpdates(Updates);

if (BBKillable)
DeleteDeadBlock(BB, DTU);
DeleteDeadBlock(BB, DTU);

return true;
}
Expand Down
4 changes: 0 additions & 4 deletions llvm/test/CodeGen/ARM/jump-table-islands.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

%BigInt = type i8500

declare void @use(%BigInt)

define %BigInt @test_moved_jumptable(i1 %tst, i32 %sw, %BigInt %l) {
; CHECK-LABEL: test_moved_jumptable:

Expand Down Expand Up @@ -36,8 +34,6 @@ other:

end:
%val = phi %BigInt [ %l, %complex ], [ -1, %simple ]
; Prevent SimplifyCFG from simplifying the phi node above.
call void @use(%BigInt %val)
ret %BigInt %val
}

Expand Down
48 changes: 27 additions & 21 deletions llvm/test/Transforms/JumpThreading/codesize-loop.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,16 @@ define i32 @test_minsize(i32 %argc, ptr nocapture readonly %argv) local_unnamed_
; OVERIDE-NEXT: [[TMP3:%.*]] = mul i32 [[CALL]], [[TMP2]]
; OVERIDE-NEXT: [[TMP4:%.*]] = icmp sgt i32 [[CALL]], 0
; OVERIDE-NEXT: [[COND_FR:%.*]] = freeze i1 [[TMP4]]
; OVERIDE-NEXT: br i1 [[COND_FR]], label [[TMP5:%.*]], label [[COND_END_THREAD]]
; OVERIDE: 5:
; OVERIDE-NEXT: br label [[COND_END_THREAD]]
; OVERIDE-NEXT: br i1 [[COND_FR]], label [[COND_END_THREAD]], label [[TMP6:%.*]]
; OVERIDE: cond.end.thread:
; OVERIDE-NEXT: [[TMP6:%.*]] = phi i32 [ [[TMP3]], [[COND_END]] ], [ [[TMP3]], [[TMP5]] ], [ 205962976, [[ENTRY:%.*]] ]
; OVERIDE-NEXT: [[TMP7:%.*]] = phi i32 [ 0, [[COND_END]] ], [ [[CALL]], [[TMP5]] ], [ 46, [[ENTRY]] ]
; OVERIDE-NEXT: [[TMP8:%.*]] = mul i32 [[TMP6]], [[TMP7]]
; OVERIDE-NEXT: [[CALL33:%.*]] = tail call i32 (ptr, ...) @printf(ptr nonnull dereferenceable(1) @.str, i32 [[TMP8]])
; OVERIDE-NEXT: [[TMP5:%.*]] = phi i32 [ [[TMP3]], [[COND_END]] ], [ 205962976, [[ENTRY:%.*]] ]
; OVERIDE-NEXT: [[COND3:%.*]] = phi i32 [ [[CALL]], [[COND_END]] ], [ 46, [[ENTRY]] ]
; OVERIDE-NEXT: br label [[TMP6]]
; OVERIDE: 6:
; OVERIDE-NEXT: [[TMP7:%.*]] = phi i32 [ [[TMP5]], [[COND_END_THREAD]] ], [ [[TMP3]], [[COND_END]] ]
; OVERIDE-NEXT: [[TMP8:%.*]] = phi i32 [ [[COND3]], [[COND_END_THREAD]] ], [ 0, [[COND_END]] ]
; OVERIDE-NEXT: [[TMP9:%.*]] = mul i32 [[TMP7]], [[TMP8]]
; OVERIDE-NEXT: [[CALL33:%.*]] = tail call i32 (ptr, ...) @printf(ptr nonnull dereferenceable(1) @.str, i32 [[TMP9]])
; OVERIDE-NEXT: ret i32 0
;
entry:
Expand Down Expand Up @@ -88,14 +90,16 @@ define i32 @test_optsize(i32 %argc, ptr nocapture readonly %argv) local_unnamed_
; DEFAULT-NEXT: [[TMP3:%.*]] = mul i32 [[CALL]], [[TMP2]]
; DEFAULT-NEXT: [[TMP4:%.*]] = icmp sgt i32 [[CALL]], 0
; DEFAULT-NEXT: [[COND_FR:%.*]] = freeze i1 [[TMP4]]
; DEFAULT-NEXT: br i1 [[COND_FR]], label [[TMP5:%.*]], label [[COND_END_THREAD]]
; DEFAULT: 5:
; DEFAULT-NEXT: br label [[COND_END_THREAD]]
; DEFAULT-NEXT: br i1 [[COND_FR]], label [[COND_END_THREAD]], label [[TMP6:%.*]]
; DEFAULT: cond.end.thread:
; DEFAULT-NEXT: [[TMP6:%.*]] = phi i32 [ [[TMP3]], [[COND_END]] ], [ [[TMP3]], [[TMP5]] ], [ 205962976, [[ENTRY:%.*]] ]
; DEFAULT-NEXT: [[TMP7:%.*]] = phi i32 [ 0, [[COND_END]] ], [ [[CALL]], [[TMP5]] ], [ 46, [[ENTRY]] ]
; DEFAULT-NEXT: [[TMP8:%.*]] = mul i32 [[TMP6]], [[TMP7]]
; DEFAULT-NEXT: [[CALL33:%.*]] = tail call i32 (ptr, ...) @printf(ptr nonnull dereferenceable(1) @.str, i32 [[TMP8]])
; DEFAULT-NEXT: [[TMP5:%.*]] = phi i32 [ [[TMP3]], [[COND_END]] ], [ 205962976, [[ENTRY:%.*]] ]
; DEFAULT-NEXT: [[COND3:%.*]] = phi i32 [ [[CALL]], [[COND_END]] ], [ 46, [[ENTRY]] ]
; DEFAULT-NEXT: br label [[TMP6]]
; DEFAULT: 6:
; DEFAULT-NEXT: [[TMP7:%.*]] = phi i32 [ [[TMP5]], [[COND_END_THREAD]] ], [ [[TMP3]], [[COND_END]] ]
; DEFAULT-NEXT: [[TMP8:%.*]] = phi i32 [ [[COND3]], [[COND_END_THREAD]] ], [ 0, [[COND_END]] ]
; DEFAULT-NEXT: [[TMP9:%.*]] = mul i32 [[TMP7]], [[TMP8]]
; DEFAULT-NEXT: [[CALL33:%.*]] = tail call i32 (ptr, ...) @printf(ptr nonnull dereferenceable(1) @.str, i32 [[TMP9]])
; DEFAULT-NEXT: ret i32 0
;
; OVERIDE-LABEL: @test_optsize(
Expand All @@ -111,14 +115,16 @@ define i32 @test_optsize(i32 %argc, ptr nocapture readonly %argv) local_unnamed_
; OVERIDE-NEXT: [[TMP3:%.*]] = mul i32 [[CALL]], [[TMP2]]
; OVERIDE-NEXT: [[TMP4:%.*]] = icmp sgt i32 [[CALL]], 0
; OVERIDE-NEXT: [[COND_FR:%.*]] = freeze i1 [[TMP4]]
; OVERIDE-NEXT: br i1 [[COND_FR]], label [[TMP5:%.*]], label [[COND_END_THREAD]]
; OVERIDE: 5:
; OVERIDE-NEXT: br label [[COND_END_THREAD]]
; OVERIDE-NEXT: br i1 [[COND_FR]], label [[COND_END_THREAD]], label [[TMP6:%.*]]
; OVERIDE: cond.end.thread:
; OVERIDE-NEXT: [[TMP6:%.*]] = phi i32 [ [[TMP3]], [[COND_END]] ], [ [[TMP3]], [[TMP5]] ], [ 205962976, [[ENTRY:%.*]] ]
; OVERIDE-NEXT: [[TMP7:%.*]] = phi i32 [ 0, [[COND_END]] ], [ [[CALL]], [[TMP5]] ], [ 46, [[ENTRY]] ]
; OVERIDE-NEXT: [[TMP8:%.*]] = mul i32 [[TMP6]], [[TMP7]]
; OVERIDE-NEXT: [[CALL33:%.*]] = tail call i32 (ptr, ...) @printf(ptr nonnull dereferenceable(1) @.str, i32 [[TMP8]])
; OVERIDE-NEXT: [[TMP5:%.*]] = phi i32 [ [[TMP3]], [[COND_END]] ], [ 205962976, [[ENTRY:%.*]] ]
; OVERIDE-NEXT: [[COND3:%.*]] = phi i32 [ [[CALL]], [[COND_END]] ], [ 46, [[ENTRY]] ]
; OVERIDE-NEXT: br label [[TMP6]]
; OVERIDE: 6:
; OVERIDE-NEXT: [[TMP7:%.*]] = phi i32 [ [[TMP5]], [[COND_END_THREAD]] ], [ [[TMP3]], [[COND_END]] ]
; OVERIDE-NEXT: [[TMP8:%.*]] = phi i32 [ [[COND3]], [[COND_END_THREAD]] ], [ 0, [[COND_END]] ]
; OVERIDE-NEXT: [[TMP9:%.*]] = mul i32 [[TMP7]], [[TMP8]]
; OVERIDE-NEXT: [[CALL33:%.*]] = tail call i32 (ptr, ...) @printf(ptr nonnull dereferenceable(1) @.str, i32 [[TMP9]])
; OVERIDE-NEXT: ret i32 0
;
entry:
Expand Down

0 comments on commit 431969e

Please sign in to comment.