Skip to content

egraphs: disable GVN of effectful idempotent ops (temporarily).#5808

Merged
cfallin merged 1 commit intobytecodealliance:mainfrom
cfallin:disable-gvn-of-effectful-ops
Feb 16, 2023
Merged

egraphs: disable GVN of effectful idempotent ops (temporarily).#5808
cfallin merged 1 commit intobytecodealliance:mainfrom
cfallin:disable-gvn-of-effectful-ops

Conversation

@cfallin
Copy link
Copy Markdown
Member

@cfallin cfallin commented Feb 16, 2023

This is a short-term fix to the same bug that #5800 is addressing (#5796), but with less risk: it simply turns off GVN'ing of effectful but idempotent ops. Because we have an upcoming release, and this is a miscompile (albeit to do with trapping behavior), we would like to make the simplest possible fix that avoids the bug, and backport it. I will then rebase #5800 on top of a revert of this followed by the more complete fix.

This deletes the test that ensures that multiple dynamically-checked heap accesses are GVN'd, because removing this functionality in fact makes that no longer the case. It will come back with the revert and proper fix!

This is a short-term fix to the same bug that bytecodealliance#5800 is addressing
(bytecodealliance#5796), but with less risk: it simply turns off GVN'ing of effectful
but idempotent ops. Because we have an upcoming release, and this is a
miscompile (albeit to do with trapping behavior), we would like to make
the simplest possible fix that avoids the bug, and backport it. I will
then rebase bytecodealliance#5800 on top of a revert of this followed by the more
complete fix.
@cfallin
Copy link
Copy Markdown
Member Author

cfallin commented Feb 16, 2023

As a procedural note, we plan to get this in and backport it, and let main sit with this while holding off any merge of #5800 (the complete fix) until after the 6.0 release, because we want to maximize fuzzing time on the released state.

Copy link
Copy Markdown
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've compared this to the result of git show -R 704f5a5 and although the comments are a little different the logic is correctly reverted.

Copy link
Copy Markdown
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nearly a revert of #5594, minus the tracing that pr introduced. Thanks!

@cfallin cfallin enabled auto-merge February 16, 2023 21:13
@cfallin cfallin added this pull request to the merge queue Feb 16, 2023
Merged via the queue into bytecodealliance:main with commit c7e2571 Feb 16, 2023
@cfallin cfallin deleted the disable-gvn-of-effectful-ops branch February 16, 2023 23:02
cfallin added a commit to cfallin/wasmtime that referenced this pull request Feb 16, 2023
cfallin added a commit that referenced this pull request Mar 2, 2023
* Revert "egraphs: disable GVN of effectful idempotent ops (temporarily). (#5808)"

This reverts commit c7e2571.

* egraphs: fix handling of effectful-but-idempotent ops and GVN.

This PR addresses #5796: currently, ops that are effectful, i.e., remain
in the side-effecting skeleton (which we keep in the `Layout` while the
egraph exists), but are idempotent and thus mergeable by a GVN pass, are
not handled properly.

GVN is still possible on effectful but idempotent ops precisely because
our GVN does not create partial redundancies: it removes an instruction
only when it is dominated by an identical instruction. An isntruction
will not be "hoisted" to a point where it could execute in the optimized
code but not in the original.

However, there are really two parts to the egraph implementation that
produce this effect: the deduplication on insertion into the egraph, and
the elaboration with a scoped hashmap. The deduplication lets us give a
single name (value ID) to all copies of an identical instruction, and
then elaboration will re-create duplicates if GVN should not hoist or
merge some of them.

Because deduplication need not worry about dominance or scopes, we use a
simple (non-scoped) hashmap to dedup/intern ops as "egraph nodes".

When we added support for GVN'ing effectful but idempotent ops (#5594),
we kept the use of this simple dedup'ing hashmap, but these ops do not
get elaborated; instead they stay in the side-effecting skeleton. Thus,
we inadvertently created potential for weird code-motion effects.

The proposal in #5796 would solve this in a clean way by treating these
ops as pure again, and keeping them out of the skeleton, instead putting
"force" pseudo-ops in the skeleton. However, this is a little more
complex than I would like, and I've realized that @jameysharp's earlier
suggestion is much simpler: we can keep an actual scoped hashmap
separately just for the effectful-but-idempotent ops, and use it to GVN
while we build the egraph. In effect, we're fusing a separate GVN pass
with the egraph pass (but letting it interact corecursively with
egraph rewrites. This is in principle similar to how we keep a separate
map for loads and fuse this pass with the egraph rewrite pass as well.

Note that we can use a `ScopedHashMap` here without the "context" (as
needed by `CtxHashMap`) because, as noted by @jameysharp, in practice
the ops we want to GVN have all their args inline. Equality on the
`InstructinoData` itself is conservative: two insts whose struct
contents compare shallowly equal are definitely identical, but identical
insts in a deep-equality sense may not compare shallowly equal, due to
list indirection. This is fine for GVN, because it is still sound to
skip any given GVN opportunity (and keep the original instructions).

Fixes #5796.

* Add comments from review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants