Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New structurizer: now with ∞% more φ! #287

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Nov 30, 2020

Not sure this is ready, but it seems to handle everything I've thrown at it.

In an effort to test it, I came up with this silly example:

(click to open example code + graph rendering)
pub fn marker() {}

pub fn shortcircuit_and(a: bool, b: bool) {
    if a && b {
        marker();
    }
}

pub fn nested_while_loop(a: bool, b: bool) {
    while a {
        let mut i = 0;
        loop {
            marker();
            match i {
                0 | 1 | 2 | 4 => {}
                5 => break,
                _ => continue,
            }
            marker();
            i += 1;
        }
        if a & b {
            break;
        }
        if a | b {
            return;
        }
        while b {
            marker();
        }
    }
}

I was inspired by the loop-specific solution of "threading all exits through one break (with a tag indicating which to take when outside the loop)" and tried to extend it to handle non-structural acyclic branches as well - the result is this PR.

You can see all the OpPhis, which are used to decide whether a "deferred exit" executes, when it's finally "fully accounted for" (or "owned"? not sure what the best terminology is) by a "region".

Most of the implementation is a SEME -> SESE propagation (with the aforementioned "deferred exits"), which is then used to handle both acyclic and cyclic "regions".

@eddyb eddyb requested review from khyperia and VZout November 30, 2020 13:53
Copy link
Contributor

@khyperia khyperia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not believe I am qualified to review new_structurizer.rs, nor am I going to try to other than a quick glance-over, because wow okay that's a lot of algorithm. Other than that, though, I think this is good to merge! (not hitting the approve button because mergify scares me)

crates/rustc_codegen_spirv/src/linker/mod.rs Outdated Show resolved Hide resolved
@@ -70,6 +80,7 @@ pub fn block_ordering_pass(func: &mut Function) {
assert_eq!(func.blocks[0].label_id().unwrap(), entry_label);
}

// FIXME(eddyb) use `Either`, `Cow`, and/or `SmallVec`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 ✅ 🆗 💯

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either<Cow, SmallVec>

Copy link
Member

@VZout VZout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume cargo test was successful. Seems way better than my code& algorithm. Haven't ran spirv-cfg to see if I can see any weird things.

crates/rustc_codegen_spirv/src/linker/mod.rs Outdated Show resolved Hide resolved
@eddyb eddyb marked this pull request as ready for review December 2, 2020 10:32
#[test]
#[ignore]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heck yee, love yeeting #[ignore]s

@mergify mergify bot merged commit e02bead into EmbarkStudios:main Dec 2, 2020
@eddyb eddyb deleted the new-structurizer branch December 2, 2020 11:05
@khyperia khyperia mentioned this pull request Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants