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

cranelift: Split predecessor/successor lists #8503

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

jameysharp
Copy link
Contributor

This establishes the property that the VCode's various lists of ranges each fully cover the index range of another list. Previously, the block_succ_range list covered the first half of block_succs_preds, and the block_pred_range list covered the second half.

While I was in the area, I replaced the O(n log n) sort in compute_preds_from_succs with a linear-time counting sort, which uses less temporary storage and directly computes the ranges we want as a byproduct.

This establishes the property that the VCode's various lists of ranges
each fully cover the index range of another list. Previously, the
block_succ_range list covered the first half of block_succs_preds, and
the block_pred_range list covered the second half.

While I was in the area, I replaced the O(n log n) sort in
compute_preds_from_succs with a linear-time counting sort, which uses
less temporary storage and directly computes the ranges we want as a
byproduct.
@jameysharp jameysharp requested a review from a team as a code owner April 29, 2024 17:59
@jameysharp jameysharp requested review from abrown and removed request for a team April 29, 2024 17:59
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

This is super-cool! IMHO this is the "Book" algorithm (in the Paul Erdos sense) for finding preds from succs. Clean and direct.

Out of curiosity have you measured the compile-time impact of this? I wonder if it'll show up in code with complex CFGs... we should take it in any case though.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. labels Apr 29, 2024
@jameysharp jameysharp added this pull request to the merge queue Apr 29, 2024
Merged via the queue into bytecodealliance:main with commit 0f4c0d4 Apr 29, 2024
21 checks passed
@jameysharp jameysharp deleted the vcode-split-preds branch April 29, 2024 20:10
@jameysharp
Copy link
Contributor Author

I hadn't measured, but according to Hyperfine, compiling the Spidermonkey benchmark from Sightglass is unchanged with this commit. I think that's unsurprising; the largest function has 2650 blocks and 6777 edges (before legalization and critical-edge splitting), and that's just not that many. But I'm still absurdly pleased with finding a situation where a linear-time sort is actually the best choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants