Skip to content

[experiment] add loop deletion llvm pass #79774

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

Closed
wants to merge 2 commits into from

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Dec 6, 2020

My findings in #79308 suggest that adding one extra loop deletion pass in the right place can eliminate some loops such that the algorithmic complexity of a function improves categorically. Adding it via -Cpasses doesn't quite work on its own (at least for x86_64 defaults), probably due to pass ordering. So this PR just hacks it directly into the llvm pass manager builder.
I'm probably not doing this the right way, but locally that's the way that requires the fewest extra passes to get the desired assembly.

If the checks pass I'd like a perf run to see what the impact is.

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2020
@the8472 the8472 marked this pull request as draft December 6, 2020 22:53
@wesleywiser
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Dec 7, 2020

⌛ Trying commit 3f1d957 with merge a89aa1b166f929b236fd540bdca9e9ae44473760...

@bors
Copy link
Collaborator

bors commented Dec 7, 2020

☀️ Try build successful - checks-actions
Build commit: a89aa1b166f929b236fd540bdca9e9ae44473760 (a89aa1b166f929b236fd540bdca9e9ae44473760)

@rust-timer
Copy link
Collaborator

Queued a89aa1b166f929b236fd540bdca9e9ae44473760 with parent 0f6f2d6, future comparison URL.

@camelid camelid added the S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. label Dec 7, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (a89aa1b166f929b236fd540bdca9e9ae44473760): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@the8472
Copy link
Member Author

the8472 commented Dec 7, 2020

That's more one-sided that I expected, but oh well. 😓

@the8472 the8472 closed this Dec 7, 2020
@AngelicosPhosphoros
Copy link
Contributor

@the8472 We measure compiler in perf here and it is obviously became slower when we use more optimization passes because it is more work to do.

Maybe we need to benchmark some another program to see the difference?

@the8472
Copy link
Member Author

the8472 commented Jul 19, 2022

@AngelicosPhosphoros it also made doc and check builds slower for some reason. They don't generate code, they only exercise rust programs. So unless I'm missing something it appears that it made rust code worse.
I don't know much about llvm internals, so it's possible that I have put it in the wrong place or it needs to be followed by some other cleanup pass to bring the IR into an ideal shape after deleting stuff, idk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants