Skip to content

Commit

Permalink
regalloc: Always rebuild CFG after inserting instructions
Browse files Browse the repository at this point in the history
Summary:
The lack of rebuilding caused a bug in the code that inserted loads for param
register in their immediate dominators. Roughly, we started with this code:

  B0:
    load-param v16
    load-param v17
    load-param v18
    load-param v19
    if-eqz v16 :foo
  B1:
    if-eqz v17 :bar

In the first iteration of the regalloc loop, we inserted a load for v18 in B1.
Without rebuilding the CFG, we end up with:

  B0:
    load-param v16
    load-param v17
    load-param v18
    load-param v19
    if-eqz v16 :foo
    move v0 v18
  B1:
    if-eqz v17 :bar

Now in the second iteration of the regalloc loop, we wanted to insert a load
for v19 in B0. We looked to see if the last instruction was a branch opcode --
that would necessitate inserting the move instruction before the branch -- but
since the last opcode in B0 is now a move, we inserted our load in the wrong
place.

I'm not sure why the ART verifier doesn't choke on this, but I'm glad arnaudvenet's
IRTypeChecker did...

Reviewed By: kgsharma

Differential Revision: D6194764

fbshipit-source-id: f7945af4771a82afc6c65f9839b84d34dfc18f15
  • Loading branch information
int3 authored and facebook-github-bot committed Oct 31, 2017
1 parent 5aaa78a commit 6a82b7b
Showing 1 changed file with 4 additions and 3 deletions.
7 changes: 4 additions & 3 deletions opt/regalloc/GraphColoring.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1099,10 +1099,11 @@ void Allocator::allocate(bool use_splitting, IRCode* code) {
TRACE(REG, 5, "Split plan:\n%s\n", SHOW(split_plan));
m_stats.split_moves +=
split(fixpoint_iter, split_plan, split_costs, ig, code);
// Since in split we might have inserted new blocks to load between
// blocks. So we call build_cfg() again to have a correct cfg.
code->build_cfg();
}

// Since we have inserted instructions, we need to rebuild the CFG to
// ensure that block boundaries remain correct
code->build_cfg();
} else {
transform::remap_registers(code, reg_transform.map);
code->set_registers_size(reg_transform.size);
Expand Down

0 comments on commit 6a82b7b

Please sign in to comment.