Wasmtime GitHub notifications bot (Jan 18 2023 at 02:08): Wasmtime GitHub notifications bot (Jan 18 2023 at 02:08):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!).
Wasmtime GitHub notifications bot (Jan 18 2023 at 02:08):cfallin requested elliottt for a review on PR #5587.
Wasmtime GitHub notifications bot (Jan 18 2023 at 02:08):cfallin requested fitzgen for a review on PR #5587.
Wasmtime GitHub notifications bot (Jan 18 2023 at 02:08):cfallin requested jameysharp for a review on PR #5587.
Wasmtime GitHub notifications bot (Jan 18 2023 at 03:04):cfallin updated PR #5587 from enable-egraphs to main.
Wasmtime GitHub notifications bot (Jan 18 2023 at 08:02):cfallin updated PR #5587 from enable-egraphs to main.
Wasmtime GitHub notifications bot (Jan 18 2023 at 17:08):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
Wasmtime GitHub notifications bot (Jan 19 2023 at 19:59):cfallin updated PR #5587 from enable-egraphs to main.
Wasmtime GitHub notifications bot (Jan 19 2023 at 19:59):cfallin has marked PR #5587 as ready for review.
Wasmtime GitHub notifications bot (Jan 19 2023 at 20:36):cfallin updated PR #5587 from enable-egraphs to main.
Wasmtime GitHub notifications bot (Jan 19 2023 at 22:21):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jan 19 2023 at 22:21):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jan 19 2023 at 22:21):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
rustcto understand the deprecation and emit a warning as well.
Wasmtime GitHub notifications bot (Jan 19 2023 at 22:21):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.
Wasmtime GitHub notifications bot (Jan 19 2023 at 22:29):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 19 2023 at 22:29):cfallin created PR review comment:
Indeed; this test and the corresponding static case were actually duplicated in #5594 to
-egraph.watcopies of the files, with egraphs enabled. The point here is to keep coverage for both cases, until we delete the non-egraphs case.
Wasmtime GitHub notifications bot (Jan 19 2023 at 22:32):cfallin updated PR #5587 from enable-egraphs to main.
Wasmtime GitHub notifications bot (Jan 19 2023 at 22:32):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 19 2023 at 22:32):cfallin created PR review comment:
Ah,nice, done!
Wasmtime GitHub notifications bot (Jan 19 2023 at 23:01):cfallin updated PR #5587 from enable-egraphs to main.
Wasmtime GitHub notifications bot (Jan 19 2023 at 23:46):cfallin merged PR #5587.
Last updated: Oct 23 2024 at 20:03 UTC