Simplify control flow for CFGs generated by eggcc.

[ezrosent]

There's a fairly detailed doc comment in simplify_branches.rs. The goal here is to clean up the CFG output and remove redundant branches from the generated code. Here's an example: at head, the sqrt benchmark generates the following cfg:

And with this PR, we get:

[ezrosent]

Question for reviewers: this will remove more branches if we iterate to a fixed point. It's easy to add; let me know if you want to try it.

[ezrosent]

Went ahead and added that in the next commit. Figure it's worth looking at nightly with everything. It's a small change and easy to revert. Here's the result for the same sqrt program.

[oflatt]

Really great job, excited that this is working!
I think some of the gnarly checks that actually do the graph rewrites can be cleaned up. The actual analysis seems pretty clean.

[oflatt]

Worth commenting intuition about Bot and Top. I think Bot is "knows nothing" and Top is unreachable because we found a conflict

[oflatt]

As far as I can tell this is the variables written to in transforms. Could we just initialize it after step 2?

[ezrosent]

We can, but then if we added more calls to add_assignment later, we'd then have to remember to recompute this. I think it's a bit less brittle to keep something like the current structure.

[oflatt]

If we keep the continue and break statements, might be worth labeling the while loops

[ezrosent]

When I see labelled while loops I generally assume that some code is going to do a multi-level break... I think the continues and breaks here are pretty clear? Are we doing this elsewhere in the code?

[oflatt]

I think this if is a special, easy case- can it be broken out into a helper? Or a whole different pass perhaps?

[ezrosent]

The thing that makes that harder is the break vs. continue case. We can lift that into its own type, but that + a bunch of params creates a bit of an odd helper. I think this might be better but I'm not sure.

As for a separate pass I think that'd require that we redo a bunch of the scaffolding for the while loops? Not sure if that's worth it either.

    enum Next { Break, Continue }
    fn fold_constant_branch(
        &mut self,
        admin_node: NodeIndex,
        succ: NodeIndex,
        arg: &Identifier,
        val_analysis: &ValueAnalysis,
        val: &Literal,
    ) -> (Next, bool) {
        if succ != self.exit && self.graph.neighbors(admin_node).any(|x| x == self.exit) {
            // Don't remove any outgoing links to the exit node.
            return (Next::Break, false);
        }
        // We assign to the branched-on argument in the admin
        // node. See if we can fold the constant branch here.
        let ValueInfo::Known(lit) = val_analysis.data[&admin_node].get_output(arg) else {
            return (Next::Continue, false);
        };
        if &lit != val {
            return (Next::Continue, false);
        }
        // okay, we have found a matching edge. Replace this branch
        // with a jump.
        let mut walker = self
            .graph
            .neighbors_directed(admin_node, Direction::Outgoing)
            .detach();
        while let Some((outgoing, _)) = walker.next(&self.graph) {
            self.graph.remove_edge(outgoing);
        }
        self.graph.add_edge(
            admin_node,
            succ,
            Branch {
                op: BranchOp::Jmp,
                pos: None,
            },
        );
        // Don't run the rest of the inner loop.
        (Next::Break, true)
}

[oflatt]

I agree the helper is weird
I still think a separate while loop may be cleaner but I'm not hung up on this

[oflatt]

Not sure I understand how scratch is getting used. Can scratch be a local definition here? What about just above this while loop instead of far up?

[ezrosent]

scratch is being used to save repeated allocations when running the algorithm, so the higher up the better I think.

I've moved it inside rewrite_branches though.

[oflatt]

Oh I see.
I think scratch is a bad name, and I don't care about the repeated allocations.

[oflatt]

Really great improvement! This looks ready to merge, it's challenging to refactor further as we've seen from the threads