Skip to content

Commit

Permalink
feat: remove orphaned blocks from cfg to improve simplify_cfg pass. (
Browse files Browse the repository at this point in the history
…#6198)

# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

Inspired by #6184, I noticed that
after simplifying a constant jmpif we wouldn't inline the block
following the inlined block into the first block, as an example consider
the SSA

```
After Dead Instruction Elimination:
brillig fn main f0 {
  b0(v0: Field, v1: Field):
    constrain v0 == Field 1
    constrain v1 == Field 0
    jmpif u1 0 then: b2, else: b1
  b2():
    v8 = add v0, Field 1
    jmp b3(v0, v8)
  b3(v2: Field, v3: Field):
    v9 = add v0, Field 1
    constrain v2 == v9
    constrain v3 == v0
    constrain v0 == Field 1
    constrain v1 == Field 0
    return 
  b1():
    v7 = add v0, Field 1
    jmp b3(v7, v0)
}
```
Here we can see that we never enter `b2`, instead going `b0 -> b1 ->
b3`. However when the simplify_cfg pass looks at the predecessors of
`b3` it sees both `b1` **and `b2`** despite `b2` not being reachable
anymore. It then doesn't inline `b3` into its sole predecessor.

On master we'll simplify the CFG for the SSA to give the program:
```
After Simplifying:
brillig fn main f0 {
  b0(v0: Field, v1: Field):
    constrain v0 == Field 1
    constrain v1 == Field 0
    v6 = add v0, Field 1
    jmp b1(v6, v0)
  b1(v2: Field, v3: Field):
    v7 = add v0, Field 1
    constrain v2 == v7
    constrain v3 == v0
    constrain v0 == Field 1
    constrain v1 == Field 0
    return 
}
```

This PR checks the previous successors of any block which has it's CFG
recomputed to check if they've been left orphaned, if so we remove it
from the CFG entirely.

We now instead fully simplify the CFG to give the program:
```
After Simplifying:
brillig fn main f0 {
  b0(v0: Field, v1: Field):
    constrain v0 == Field 1
    constrain v1 == Field 0
    v4 = add v0, Field 1
    v5 = add v0, Field 1
    constrain v4 == v5
    constrain v0 == Field 1
    constrain v1 == Field 0
    return 
}

```

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
TomAFrench authored Oct 2, 2024
1 parent 594ec91 commit b4712c5
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 4 deletions.
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/ir/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl ControlFlowGraph {

/// Clears out a given block's successors. This also removes the given block from
/// being a predecessor of any of its previous successors.
fn invalidate_block_successors(&mut self, basic_block_id: BasicBlockId) {
pub(crate) fn invalidate_block_successors(&mut self, basic_block_id: BasicBlockId) {
let node = self
.data
.get_mut(&basic_block_id)
Expand Down
14 changes: 11 additions & 3 deletions compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ impl Function {
check_for_constant_jmpif(self, block, &mut cfg);

let mut predecessors = cfg.predecessors(block);

if predecessors.len() == 1 {
let predecessor =
predecessors.next().expect("Already checked length of predecessors");
Expand Down Expand Up @@ -99,14 +98,23 @@ fn check_for_constant_jmpif(
}) = function.dfg[block].terminator()
{
if let Some(constant) = function.dfg.get_numeric_constant(*condition) {
let destination =
if constant.is_zero() { *else_destination } else { *then_destination };
let (destination, unchosen_destination) = if constant.is_zero() {
(*else_destination, *then_destination)
} else {
(*then_destination, *else_destination)
};

let arguments = Vec::new();
let call_stack = call_stack.clone();
let jmp = TerminatorInstruction::Jmp { destination, arguments, call_stack };
function.dfg[block].set_terminator(jmp);
cfg.recompute_block(function, block);

// If `block` was the only predecessor to `unchosen_destination` then it's no long reachable through the CFG,
// we can then invalidate it successors as it's an invalid predecessor.
if cfg.predecessors(unchosen_destination).len() == 0 {
cfg.invalidate_block_successors(unchosen_destination);
}
}
}
}
Expand Down

0 comments on commit b4712c5

Please sign in to comment.