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

structurizer: split regions into Divergent vs Convergent, and optimize based on that. #574

Closed

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Apr 1, 2021

The first commit is a pretty large refactor that ended up a bit worse than I expected.
We might just want to have this instead (and call it in a few places):

impl Region {
    fn diverges(&self) -> bool {
        self.exits.is_empty()
    }
}

The second commit is just an on-the-fly OpPhi simplification that we should probably merge completely separately.

And the third commit is "clever hack" (to remove some unnecessary empty blocks, see example below) that could also probably be done without the first commit, it just didn't hit me that it was possible before the refactor.

(click to open example code & before/after graphs for the third commit)
pub fn marker() {}

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

Before

After

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.

Seems good to me! Type saftey with different types for divergent/convergent is probably kinda nice, makes you think about things instead of writing generic code for both, so I'd probably be in favor of keeping it if you like it.

merge_id: block_id,
exits: indexmap! {},
},
Op::Return | Op::ReturnValue | Op::Kill | Op::Unreachable => Region::Divergent,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note here that this will conflict with #563, and it might not be immediately obvious if a merge goes wonky here, so note to double check a merge goes well

@eddyb eddyb added the s: waiting on author PRs that blocked on the author implementing feedback. label May 21, 2022
@eddyb
Copy link
Contributor Author

eddyb commented Feb 14, 2023

Closing in favor of SPIR-T (which has its own structurizer, with a different design).

@eddyb eddyb closed this Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: waiting on author PRs that blocked on the author implementing feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants