[llvm] 78af5cb - [SimplifyCFG] performBranchToCommonDestFolding(): form block-closed SSA form before cloning instructions (PR51125)
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Sun Aug 15 09:05:54 PDT 2021
Author: Roman Lebedev
Date: 2021-08-15T19:02:34+03:00
New Revision: 78af5cb213b2f9fe3f47bf23947f14ac07024155
URL: https://github.com/llvm/llvm-project/commit/78af5cb213b2f9fe3f47bf23947f14ac07024155
DIFF: https://github.com/llvm/llvm-project/commit/78af5cb213b2f9fe3f47bf23947f14ac07024155.diff
LOG: [SimplifyCFG] performBranchToCommonDestFolding(): form block-closed SSA form before cloning instructions (PR51125)
LLVM IR SSA form is "implicit" in `@pr51125`. While is a valid LLVM IR,
and does not require any PHI nodes, that completely breaks the further logic
in `CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses()`
that updates the live-out uses of the bonus instructions.
What i believe we need to do, is to first make the SSA form explicit,
by inserting tautological PHI nodes, and rewriting the offending uses.
```
$ /builddirs/llvm-project/build-Clang12/bin/opt -load /repositories/alive2/build-Clang-release/tv/tv.so -load-pass-plugin /repositories/alive2/build-Clang-release/tv/tv.so -tv -simplifycfg -simplifycfg-require-and-preserve-domtree=1 -bonus-inst-threshold=10 -tv -o /dev/null /tmp/test.ll
----------------------------------------
@global_pr51125 = global 4 bytes, align 4
define i32 @pr51125() {
%entry:
br label %L
%L:
%ld = load i32, * @global_pr51125, align 4
%iszero = icmp eq i32 %ld, 0
br i1 %iszero, label %exit, label %L2
%L2:
store i32 4294967295, * @global_pr51125, align 4
%cmp = icmp eq i32 %ld, 4294967295
br i1 %cmp, label %L, label %exit
%exit:
%r = phi i32 [ %ld, %L2 ], [ %ld, %L ]
ret i32 %r
}
=>
@global_pr51125 = global 4 bytes, align 4
define i32 @pr51125() {
%entry:
%ld.old = load i32, * @global_pr51125, align 4
%iszero.old = icmp eq i32 %ld.old, 0
br i1 %iszero.old, label %exit, label %L2
%L2:
%ld2 = phi i32 [ %ld.old, %entry ], [ %ld, %L2 ]
store i32 4294967295, * @global_pr51125, align 4
%cmp = icmp ne i32 %ld2, 4294967295
%ld = load i32, * @global_pr51125, align 4
%iszero = icmp eq i32 %ld, 0
%or.cond = select i1 %cmp, i1 1, i1 %iszero
br i1 %or.cond, label %exit, label %L2
%exit:
%ld1 = phi i32 [ poison, %L2 ], [ %ld.old, %entry ]
%r = phi i32 [ %ld2, %L2 ], [ %ld.old, %entry ]
ret i32 %r
}
Transformation seems to be correct!
```
Fixes https://bugs.llvm.org/show_bug.cgi?id=51125
Reviewed By: nikic
Differential Revision: https://reviews.llvm.org/D106317
Added:
Modified:
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 847fdd760d2f..68a0388398fc 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1095,17 +1095,24 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
// Update (liveout) uses of bonus instructions,
// now that the bonus instruction has been cloned into predecessor.
- SSAUpdater SSAUpdate;
- SSAUpdate.Initialize(BonusInst.getType(),
- (NewBonusInst->getName() + ".merge").str());
- SSAUpdate.AddAvailableValue(BB, &BonusInst);
- SSAUpdate.AddAvailableValue(PredBlock, NewBonusInst);
+ // Note that we expect to be in a block-closed SSA form for this to work!
for (Use &U : make_early_inc_range(BonusInst.uses())) {
auto *UI = cast<Instruction>(U.getUser());
- if (UI->getParent() != PredBlock)
- SSAUpdate.RewriteUseAfterInsertions(U);
- else // Use is in the same block as, and comes before, NewBonusInst.
- SSAUpdate.RewriteUse(U);
+ auto *PN = dyn_cast<PHINode>(UI);
+ if (!PN) {
+ assert(UI->getParent() == BB && BonusInst.comesBefore(UI) &&
+ "If the user is not a PHI node, then it should be in the same "
+ "block as, and come after, the original bonus instruction.");
+ continue; // Keep using the original bonus instruction.
+ }
+ // Is this the block-closed SSA form PHI node?
+ if (PN->getIncomingBlock(U) == BB)
+ continue; // Great, keep using the original bonus instruction.
+ // The only other alternative is an "use" when coming from
+ // the predecessor block - here we should refer to the cloned bonus instr.
+ assert(PN->getIncomingBlock(U) == PredBlock &&
+ "Not in block-closed SSA form?");
+ U.set(NewBonusInst);
}
}
}
@@ -3032,6 +3039,56 @@ static bool performBranchToCommonDestFolding(BranchInst *BI, BranchInst *PBI,
LLVM_DEBUG(dbgs() << "FOLDING BRANCH TO COMMON DEST:\n" << *PBI << *BB);
+ // We want to duplicate all the bonus instructions in this block,
+ // and rewrite their uses, but in some cases with self-loops,
+ // the naive use rewrite approach won't work (will result in miscompilations).
+ // To avoid this problem, let's form block-closed SSA form.
+ for (Instruction &BonusInst :
+ reverse(iterator_range<BasicBlock::iterator>(*BB))) {
+ auto IsBCSSAUse = [BB, &BonusInst](Use &U) {
+ auto *UI = cast<Instruction>(U.getUser());
+ if (auto *PN = dyn_cast<PHINode>(UI))
+ return PN->getIncomingBlock(U) == BB;
+ return UI->getParent() == BB && BonusInst.comesBefore(UI);
+ };
+
+ // Does this instruction require rewriting of uses?
+ if (all_of(BonusInst.uses(), IsBCSSAUse))
+ continue;
+
+ SSAUpdater SSAUpdate;
+ Type *Ty = BonusInst.getType();
+ SmallVector<PHINode *, 8> BCSSAPHIs;
+ SSAUpdate.Initialize(Ty, BonusInst.getName());
+
+ // Into each successor block of BB, insert a PHI node, that receives
+ // the BonusInst when coming from it's basic block, or poison otherwise.
+ for (BasicBlock *Succ : successors(BB)) {
+ // The block may have the same successor multiple times. Do it only once.
+ if (SSAUpdate.HasValueForBlock(Succ))
+ continue;
+ BCSSAPHIs.emplace_back(PHINode::Create(
+ Ty, 0, BonusInst.getName() + ".bcssa", &Succ->front()));
+ PHINode *PN = BCSSAPHIs.back();
+ for (BasicBlock *PredOfSucc : predecessors(Succ))
+ PN->addIncoming(PredOfSucc == BB ? (Value *)&BonusInst
+ : PoisonValue::get(Ty),
+ PredOfSucc);
+ SSAUpdate.AddAvailableValue(Succ, PN);
+ }
+
+ // And rewrite all uses that break block-closed SSA form.
+ for (Use &U : make_early_inc_range(BonusInst.uses()))
+ if (!IsBCSSAUse(U))
+ SSAUpdate.RewriteUseAfterInsertions(U);
+
+ // We might not have ended up needing PHI's in all of the succ blocks,
+ // drop the ones that are certainly unused, but don't bother otherwise.
+ for (PHINode *PN : BCSSAPHIs)
+ if (PN->use_empty())
+ PN->eraseFromParent();
+ }
+
IRBuilder<> Builder(PBI);
// The builder is used to create instructions to eliminate the branch in BB.
// If BB's terminator has !annotation metadata, add it to the new
More information about the llvm-commits
mailing list