cfallin opened PR #5587 from enable-egraphs
to main
:
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!).
cfallin requested elliottt for a review on PR #5587.
cfallin requested fitzgen for a review on PR #5587.
cfallin requested jameysharp for a review on PR #5587.
cfallin updated PR #5587 from enable-egraphs
to main
.
cfallin updated PR #5587 from enable-egraphs
to main
.
fitzgen edited PR #5587 from enable-egraphs
to main
:
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 https://github.com/bytecodealliance/wasmtime/issues/5181
cfallin updated PR #5587 from enable-egraphs
to main
.
cfallin has marked PR #5587 as ready for review.
cfallin updated PR #5587 from enable-egraphs
to main
.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
You can add
#[deprecated(since="5.0.0", note="egraphs will be the default and this method will be removed in a future version")]
to get
rustc
to understand the deprecation and emit a warning as well.
fitzgen created PR review comment:
For this test, and the other in this commit, I think we do want to enable egraphs (or at least use the default setting). The point of these tests is to make sure that we deduplicate the loads by default, not to exercise a particular pass.
cfallin submitted PR review.
cfallin created PR review comment:
Indeed; this test and the corresponding static case were actually duplicated in #5594 to
-egraph.wat
copies of the files, with egraphs enabled. The point here is to keep coverage for both cases, until we delete the non-egraphs case.
cfallin updated PR #5587 from enable-egraphs
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
Ah,nice, done!
cfallin updated PR #5587 from enable-egraphs
to main
.
cfallin merged PR #5587.
Last updated: Dec 23 2024 at 12:05 UTC