Skip to content

Commit 7c1d72b

Browse files
alexmalyshevmeta-codesync[bot]
authored andcommitted
Replace removeTrampolineBlocks() with mergeLinearBlocks()
Summary: `removeTrampolineBlocks()` only handled merging blocks that contained snapshots and a single `Branch`. `CleanCFG` had an `absortDstBlock()` function that covered the full A -> B linear block merging step. It was kind of strange having the two separate from each other, so they're now adjacent to each other. `absorbDstBlock()` would also abort if it ever got a phi in the successor block. Technically phis are possible for the function to hit, they could be trivial phis like `B = Phi A`. We _happen_ not to hit that case because we would run `PhiElimination` right before the loop, but we shouldn't expect that to always be the case. `absortDstBlock()` also left the target block lying around, `absorbSuccessor()` on the other hand will delete it immediately. All in all this makes things a bit cleaner and hopefully easier to understand now. We do see a change in CFGs, which is interesting. It appears to be hitting `test.test_functools:TestSingleDispatch.test_mro_conflicts` and leading to us being able to optimize `LOAD_ATTR` a bit further. Probably because we're cleaning up the CFG a bit better in the builder, and the simplifier runs right afterwards and can hit one of the specialized cases instead of `LoadAttr -> LoadAttrCached`. Reviewed By: yoney Differential Revision: D114439589 fbshipit-source-id: b2ab40b0517b24afa954b36a72e234a7bdf3677d
1 parent 1759adf commit 7c1d72b

11 files changed

Lines changed: 483 additions & 248 deletions

File tree

cinderx/Jit/hir/builder.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -594,9 +594,9 @@ std::unique_ptr<Function> HIRBuilder::buildHIR() {
594594

595595
std::unique_ptr<Function> irfunc = preloader_.makeFunction();
596596
buildHIRImpl(irfunc.get(), /*frame_state=*/nullptr);
597-
// Use removeTrampolineBlocks and removeUnreachableBlocks directly instead of
598-
// Run because the rest of CleanCFG requires SSA.
599-
removeTrampolineBlocks(*irfunc);
597+
// Use mergeLinearBlocks and removeUnreachableBlocks directly instead of
598+
// CleanCFG because the rest of CleanCFG requires SSA.
599+
mergeLinearBlocks(*irfunc);
600600
removeUnreachableBlocks(*irfunc);
601601
return irfunc;
602602
}

cinderx/Jit/hir/clean_cfg.cpp

Lines changed: 21 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -2,72 +2,37 @@
22

33
#include "cinderx/Jit/hir/clean_cfg.h"
44

5+
#include "cinderx/Common/log.h"
56
#include "cinderx/Jit/hir/phi_elimination.h"
6-
#include "cinderx/Jit/hir/printer.h"
77

88
namespace cinderx::jit::hir {
99

10-
namespace {
11-
12-
bool absorbDstBlock(BasicBlock* block) {
13-
if (block->getTerminator()->opcode() != Opcode::kBranch) {
14-
return false;
15-
}
16-
auto branch = dynamic_cast<Branch*>(block->getTerminator());
17-
BasicBlock* target = branch->target();
18-
if (target == block) {
19-
return false;
20-
}
21-
if (target->inEdges().size() != 1) {
22-
return false;
23-
}
24-
if (target == block) {
25-
return false;
26-
}
27-
branch->unlink();
28-
while (!target->empty()) {
29-
Instr* instr = target->pop_front();
30-
JIT_CHECK(!instr->isPhi(), "Expected no Phi but found {}", *instr);
31-
block->append(instr);
32-
}
33-
// The successors to target might have Phis that still refer to target.
34-
// Retarget them to refer to block.
35-
Instr* old_term = block->getTerminator();
36-
JIT_CHECK(old_term != nullptr, "block must have a terminator");
37-
for (std::size_t i = 0, n = old_term->numEdges(); i < n; ++i) {
38-
old_term->successor(i)->fixupPhis(
39-
/*old_pred=*/target, /*new_pred=*/block);
40-
}
41-
// Target block becomes unreachable and gets picked up by
42-
// removeUnreachableBlocks.
43-
delete branch;
44-
return true;
45-
}
46-
47-
} // namespace
48-
4910
void CleanCFG::run(Function& irfunc) {
11+
constexpr size_t kRunLimit = 10;
12+
size_t run = 0;
5013
bool changed = false;
5114

52-
do {
15+
for (; run < kRunLimit; ++run) {
5316
removeUnreachableInstructions(irfunc);
54-
// Remove any trivial Phis; absorbDstBlock cannot handle them.
17+
// Collapse trivial Phis everywhere, not just in the blocks that get merged
18+
// below.
5519
PhiElimination{}.run(irfunc);
56-
std::vector<BasicBlock*> blocks = irfunc.cfg.getRPOTraversal();
57-
for (auto block : blocks) {
58-
// Ignore transient empty blocks.
59-
if (block->empty()) {
60-
continue;
61-
}
62-
// Keep working on the current block until no further changes are made.
63-
for (;; changed = true) {
64-
if (absorbDstBlock(block)) {
65-
continue;
66-
}
67-
break;
68-
}
20+
21+
bool modified = mergeLinearBlocks(irfunc);
22+
modified |= removeUnreachableBlocks(irfunc);
23+
changed |= modified;
24+
25+
if (!modified) {
26+
break;
6927
}
70-
} while (removeUnreachableBlocks(irfunc));
28+
}
29+
30+
JIT_THROW_IF(
31+
run == kRunLimit,
32+
"CleanCFG for function '{}' did not complete in the maximum number of "
33+
"runs ({})",
34+
irfunc.fullname,
35+
kRunLimit);
7136

7237
if (changed) {
7338
reflowTypes(irfunc);

cinderx/Jit/hir/hir.cpp

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1183,26 +1183,6 @@ Snapshot* BasicBlock::entrySnapshot() {
11831183
return nullptr;
11841184
}
11851185

1186-
bool BasicBlock::isTrampoline() {
1187-
for (auto& instr : instrs_) {
1188-
if (instr.isBranch()) {
1189-
auto succ = instr.successor(0);
1190-
// Don't consider a block a trampoline if its successor has one or more
1191-
// Phis, since this block may be necessary to pass a specific value to
1192-
// the Phi. This is correct but conservative: it's often safe to
1193-
// eliminate trampolines that jump to Phis, but that requires more
1194-
// involved analysis in the caller.
1195-
return succ != this && (succ->empty() || !succ->front().isPhi());
1196-
}
1197-
if (instr.isSnapshot()) {
1198-
continue;
1199-
}
1200-
return false;
1201-
}
1202-
// empty block
1203-
return false;
1204-
}
1205-
12061186
void BasicBlock::fixupPhis(BasicBlock* old_pred, BasicBlock* new_pred) {
12071187
// This won't work correctly if this block has two incoming edges from the
12081188
// same block, but we already can't handle that correctly with our current Phi

cinderx/Jit/hir/pass.cpp

Lines changed: 151 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,20 @@ Register* chaseAssignOperand(Register* value) {
9999
return value;
100100
}
101101

102+
Instr* collapseTrivialPhi(Phi& phi) {
103+
Register* value = phi.isTrivial();
104+
if (value == nullptr) {
105+
return nullptr;
106+
}
107+
Register* output = phi.output();
108+
// A trivial Phi that only references itself can never be initialized, so use
109+
// a LoadConst<Bottom> to signify that.
110+
if (chaseAssignOperand(value) == output) {
111+
return LoadConst::create(output, TBottom);
112+
}
113+
return Assign::create(output, value);
114+
}
115+
102116
RegUses collectDirectRegUses(Function& func) {
103117
RegUses uses;
104118
for (auto& block : func.cfg.blocks) {
@@ -616,42 +630,154 @@ void reflowTypes(Function& func, BasicBlock* start) {
616630
}
617631
}
618632

619-
void removeTrampolineBlocks(Function& func) {
620-
auto cfg = &func.cfg;
633+
namespace {
634+
635+
// Get the block that `block` unconditionally jumps to, or nullptr if it ends in
636+
// anything other than a Branch.
637+
BasicBlock* branchTarget(BasicBlock& block) {
638+
Instr* term = block.getTerminator();
639+
if (term == nullptr || term->opcode() != Opcode::kBranch) {
640+
return nullptr;
641+
}
642+
return static_cast<Branch*>(term)->target();
643+
}
644+
645+
// Absorb the sole successor of `block` into it, deleting the successor. Return
646+
// false if `block` doesn't end a linear A -> B pair.
647+
bool absorbSuccessor(Function& func, BasicBlock& block) {
648+
// Identify linear blocks, A -> B.
649+
BasicBlock* target = branchTarget(block);
650+
if (target == nullptr || target == &block || target->empty() ||
651+
target->inEdges().size() != 1) {
652+
return false;
653+
}
654+
// The entry block's instructions have to run first, so it can never be
655+
// absorbed into one of its own successors.
656+
if (target == func.cfg.entry_block) {
657+
return false;
658+
}
659+
660+
// Any phis in B are trivial because B only has one predecessor. They can't
661+
// move to the end of A as phis only live at the start of a block, so collapse
662+
// them into assignments in place.
663+
for (auto it = target->begin(); it != target->end();) {
664+
Instr& instr = *it;
665+
++it;
666+
if (!instr.isPhi()) {
667+
break;
668+
}
669+
Instr* new_instr = collapseTrivialPhi(static_cast<Phi&>(instr));
670+
JIT_THROW_IF(
671+
new_instr == nullptr,
672+
"Non-trivial Phi '{}' in bb {} of {}, which only has one predecessor",
673+
instr,
674+
target->id,
675+
func.fullname);
676+
target->replace(instr, *new_instr);
677+
delete &instr;
678+
}
679+
680+
// Drop the branch, then append all instructions from B onto A. The branch
681+
// has to go before B does, it owns the last edge pointing at B.
682+
Instr* branch = block.getTerminator();
683+
branch->unlink();
684+
delete branch;
685+
while (!target->empty()) {
686+
block.append(target->pop_front());
687+
}
688+
689+
// The successors of B might still have phis that refer to it. Retarget them
690+
// to A.
691+
Instr* new_term = block.getTerminator();
692+
for (std::size_t i = 0, n = new_term->numEdges(); i < n; ++i) {
693+
new_term->successor(i)->fixupPhis(target, &block);
694+
}
621695

622-
std::vector<BasicBlock*> trampolines;
623-
for (auto& block : cfg->blocks) {
624-
if (!block.isTrampoline()) {
696+
// B can now be deleted.
697+
func.cfg.removeBlock(target);
698+
delete target;
699+
return true;
700+
}
701+
702+
// A trampoline block does nothing but jump to another block. Snapshots are
703+
// ignored as they're only metadata.
704+
bool isTrampoline(BasicBlock& block) {
705+
for (Instr& instr : block) {
706+
if (instr.isSnapshot()) {
625707
continue;
626708
}
627-
BasicBlock* succ = block.successor(0);
628-
// if this is the entry block and its successor has multiple
629-
// predecessors, don't remove it; it's necessary to maintain isolated
630-
// entries
631-
if (&block == cfg->entry_block) {
632-
if (succ->inEdges().size() > 1) {
633-
continue;
634-
} else {
635-
cfg->entry_block = succ;
636-
}
709+
if (!instr.isBranch()) {
710+
return false;
637711
}
638-
// Update all predecessors to jump directly to our successor
639-
block.retargetPreds(succ);
640-
// Finish splicing the trampoline out of the cfg
641-
block.setSuccessor(0, nullptr);
642-
trampolines.emplace_back(&block);
712+
BasicBlock* succ = instr.successor(0);
713+
// Don't treat a block as a trampoline if its successor has Phis, this
714+
// block may be necessary to pass a specific value to one of them. That's
715+
// correct but conservative: it's often safe to eliminate such trampolines,
716+
// but it needs more involved analysis.
717+
return succ != &block && (succ->empty() || !succ->front().isPhi());
643718
}
719+
// Empty block.
720+
return false;
721+
}
644722

645-
for (auto& block : trampolines) {
646-
cfg->removeBlock(block);
647-
delete block;
723+
// Splice a trampoline block out of the CFG by pointing all of its predecessors
724+
// at its successor, deleting the block. Return false if `block` isn't a
725+
// trampoline.
726+
//
727+
// This is the other half of a linear A -> B merge: it applies when B has other
728+
// predecessors and so can't be absorbed into A.
729+
bool spliceTrampoline(Function& func, BasicBlock& block) {
730+
// Keep the entry block around, it's needed to maintain an isolated entry.
731+
// When its successor only has one predecessor absorbSuccessor() handles it.
732+
if (&block == func.cfg.entry_block || !isTrampoline(block)) {
733+
return false;
734+
}
735+
736+
block.retargetPreds(block.successor(0));
737+
block.setSuccessor(0, nullptr);
738+
func.cfg.removeBlock(&block);
739+
delete &block;
740+
return true;
741+
}
742+
743+
// Run a single merge pass over every block in the CFG, returning true if any
744+
// blocks were removed.
745+
bool mergeLinearBlocksOnce(Function& func) {
746+
bool changed = false;
747+
for (auto it = func.cfg.blocks.begin(); it != func.cfg.blocks.end();) {
748+
BasicBlock& block = *it;
749+
750+
// Keep absorbing successors, chains of them collapse into a single block.
751+
// This only ever unlinks the successor, never `block`, so the iterator
752+
// stays valid.
753+
while (absorbSuccessor(func, block)) {
754+
changed = true;
755+
}
756+
757+
// Splicing deletes `block`, so step past it first.
758+
++it;
759+
changed |= spliceTrampoline(func, block);
648760
}
761+
return changed;
762+
}
763+
764+
} // namespace
649765

650-
simplifyRedundantCondBranches(cfg);
766+
bool mergeLinearBlocks(Function& func) {
767+
bool changed = false;
768+
for (bool modified = true; modified;) {
769+
// Folding CondBranch<X, X> into Branch<X> leaves X with one fewer
770+
// predecessor, which can make it a merge candidate. Splicing trampolines
771+
// out below is what tends to create these.
772+
simplifyRedundantCondBranches(&func.cfg);
773+
modified = mergeLinearBlocksOnce(func);
774+
changed |= modified;
775+
}
651776

652-
if (trampolines.size() > 0) {
777+
if (changed) {
653778
func.invalidateDomTree();
654779
}
780+
return changed;
655781
}
656782

657783
bool removeUnreachableBlocks(Function& func) {

cinderx/Jit/hir/pass.h

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,15 @@ using RegUses = std::unordered_map<Register*, std::unordered_set<Instr*>>;
3737
// If there are no assignments then just get the register back.
3838
Register* chaseAssignOperand(Register* value);
3939

40+
// Take a phi instruction and try to collapse it into a new assignment
41+
// instruction if it is trivial (merges in only one other value). If it's not
42+
// trivial return nullptr. If it would turn into a malformed assignment
43+
// (`A = Phi A`), then return a load of TBottom instead.
44+
//
45+
// The caller owns the returned instruction and is responsible for linking it
46+
// into a block.
47+
Instr* collapseTrivialPhi(Phi& phi);
48+
4049
// Collect direct operand uses of all Registers in the given func, excluding
4150
// uses in FrameState or other metadata.
4251
RegUses collectDirectRegUses(Function& func);
@@ -59,8 +68,13 @@ Type outputType(
5968
void reflowTypes(Function& func);
6069
void reflowTypes(Function& func, BasicBlock* start);
6170

62-
// Remove any blocks that consist of a single jump to another block.
63-
void removeTrampolineBlocks(Function& func);
71+
// Combine all blocks A and B where A only has B as a successor, B only has A as
72+
// a predecessor, and A and B are distinct blocks (not cycles). Chains of such
73+
// blocks collapse down into a single block. Return true if the CFG changed.
74+
//
75+
// Any Phi at the top of B is necessarily trivial and gets collapsed into an
76+
// Assign, as Phis can only live at the start of a block.
77+
bool mergeLinearBlocks(Function& func);
6478

6579
// Remove blocks that aren't reachable from the entry, whether or not they're
6680
// empty. Return true if it changed the graph and false otherwise.

cinderx/Jit/hir/phi_elimination.cpp

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,7 @@ void PhiElimination::run(Function& func) {
2121
}
2222
break;
2323
}
24-
if (auto value = static_cast<Phi&>(instr).isTrivial()) {
25-
// If a trivial Phi references itself then it can never be
26-
// initialized, and we can use a LoadConst<Bottom> to signify that.
27-
Register* model_value = chaseAssignOperand(value);
28-
Instr* new_instr;
29-
if (model_value == instr.output()) {
30-
new_instr = LoadConst::create(instr.output(), TBottom);
31-
} else {
32-
new_instr = Assign::create(instr.output(), value);
33-
}
24+
if (auto new_instr = collapseTrivialPhi(static_cast<Phi&>(instr))) {
3425
new_instr->copyBytecodeOffset(instr);
3526
assigns_or_loads.emplace_back(new_instr);
3627
instr.unlink();
@@ -44,7 +35,7 @@ void PhiElimination::run(Function& func) {
4435
}
4536

4637
// Consider having a separate run of CleanCFG between passes clean this up.
47-
removeTrampolineBlocks(func);
38+
mergeLinearBlocks(func);
4839
}
4940

5041
} // namespace cinderx::jit::hir

0 commit comments

Comments
 (0)