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: Delete redundant DCE optimization pass #8227

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jameysharp
Copy link
Contributor

The egraph pass and the dead-code elimination pass both remove instructions whose results are unused. If the optimization level is "none", neither pass runs, and if it's anything else both passes run. I don't think we should do this work twice.

Note that the DCE pass is different than the "eliminate unreachable code" pass, which removes entire blocks that are unreachable from the entry block. That pass might still be necessary.

@cfallin, what do you think? There's some advantage to not calling simplify on instructions that we can quickly prove are unused, but is it worth keeping a redundant pass around for that?

The egraph pass and the dead-code elimination pass both remove
instructions whose results are unused. If the optimization level is
"none", neither pass runs, and if it's anything else both passes run. I
don't think we should do this work twice.

Note that the DCE pass is different than the "eliminate unreachable
code" pass, which removes entire blocks that are unreachable from the
entry block. That pass might still be necessary.
@jameysharp jameysharp requested a review from cfallin March 23, 2024 01:04
@jameysharp jameysharp requested a review from a team as a code owner March 23, 2024 01:04
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Mar 23, 2024
@cfallin
Copy link
Member

cfallin commented Mar 23, 2024

IIRC, the "legacy" passes we call before egraph opt are the ones that are important for compile time, or at least were when egraph opt was introduced. Basically as you said: better not to invoke ISLE at all if code is unused (and egraph's natural DCE comes only during elaboration after that). Would you mind measuring the compile-time impact of this? It's always possible things are better now of course!

@elliottt elliottt removed the request for review from a team March 26, 2024 16:59
@jameysharp
Copy link
Contributor Author

I finally got back to this. Sightglass says there's no impact on the smaller benchmarks (bz2, pulldown-cmark) in any phase or by any perf counter. On the Spidermonkey benchmark, there's no impact on instructions retired, but when measuring CPU cycles there's a slight but statistically significant slowdown in compilation, and weirdly, a slight speedup in execution.

I'm inclined to think this is pretty much still in the noise and we should go ahead and merge this so we have less code to maintain. What do you think?

compilation :: cpu-cycles :: benchmarks/spidermonkey/benchmark.wasm

  Δ = 2481348.89 ± 2266894.53 (confidence = 99%)

  no-dedup-dce-b545cc508.so is 1.00x to 1.02x faster than dedup-dce-8e9f6f135.so!

  [291935280 305568325.04 334203152] dedup-dce-8e9f6f135.so
  [261612097 303086976.15 312831291] no-dedup-dce-b545cc508.so

execution :: cpu-cycles :: benchmarks/spidermonkey/benchmark.wasm

  Δ = 7533948.79 ± 2750726.94 (confidence = 99%)

  dedup-dce-8e9f6f135.so is 1.00x to 1.01x faster than no-dedup-dce-b545cc508.so!

  [965718291 973510333.10 1006394505] dedup-dce-8e9f6f135.so
  [965415521 981044281.89 1015728504] no-dedup-dce-b545cc508.so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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