-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Enable egraph-based optimization by default. #5587
Conversation
692757b
to
1494936
Compare
Hmm, some issues caused by my reimplementation of #5534 (GVN + idempotency) for egraphs; switching this to draft for now. |
Pushed updates, but I'll factor out the idempotent-GVN updates for egraphs into a separate PR tomorrow then rebase this. |
Also update the pass-driving logic to run egraph optimization only when optimization is enabled. Previously `use_egraphs` acted as an overall enable-switch that drove egraph-based optimization, regardless of `opt_level`.
4a49c13
to
37f02db
Compare
Rebased on the idempotency fix; should be ready to review now! |
I think one final location to touch doc-wise would be here too perhaps? |
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "cranelift:meta", "wasmtime:api", "wasmtime:config"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
To modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
In an extremely amusing twist, adding the deprecation warning for the |
98ca402
to
a5acda5
Compare
You can |
After bytecodealliance#5587, this is on by default. We are retaining the traditional (no-egraphs) path for now, selected by setting this option to `false`, but we eventually plan to delete it assuming that we don't find serious regressions or issues. This PR adds a deprecation notice to the option.
After #5587, this is on by default. We are retaining the traditional (no-egraphs) path for now, selected by setting this option to `false`, but we eventually plan to delete it assuming that we don't find serious regressions or issues. This PR adds a deprecation notice to the option.
This PR follows up on #5382 and #5391, which rebuilt the egraph-based optimization framework to be more performant, by enabling it by default.
Based on performance results in #5382 (my measurements on SpiderMonkey and bjorn3's independent confirmation with cg_clif), it seems that this is reasonable to enable. Now that we have been fuzzing compiler configurations with egraph opts (#5388) for 6 weeks, having fixed a few fuzzbugs that came up (#5409, #5420, #5438) and subsequently received no further reports from OSS-Fuzz, I believe it is stable enough to rely on.
This PR enables
use_egraphs
, and also normalizes its meaning: previously it forced optimization (it basically meant "turn on the egraph optimization machinery"), now it runs egraph opts if the opt level indicates (it means "use egraphs to optimize if we are going to optimize"). The conditionals in the top-level pass driver are a little subtle, but will get simpler once we can remove the non-egraph path (which we plan to do eventually!).Fixes #5181