Wasmtime GitHub notifications bot (Jan 14 2025 at 01:23): Wasmtime GitHub notifications bot (Jan 14 2025 at 01:23):fitzgen opened PR #10004 from fitzgen:idempotent-trapz-trapnz to bytecodealliance:main:
This commit extends our existing support for merging idempotently side-effectful instructions that produce exactly one value to those that produce zero or one value, and marks the
trap[n]zinstructions as having idempotent side effects. This cleans up a lot test cases in ourdisastest suite, particularly those related to explicit bounds checks and GC.As an aside, it seems like it should be easy to extend this to idempotently side-effectful instructions that produce multiple values as well, but I don't believe we have any such instructions, so I didn't bother.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jan 14 2025 at 01:23):fitzgen requested cfallin for a review on PR #10004.
Wasmtime GitHub notifications bot (Jan 14 2025 at 01:23):fitzgen requested wasmtime-compiler-reviewers for a review on PR #10004.
Wasmtime GitHub notifications bot (Jan 14 2025 at 01:23):fitzgen requested dicej for a review on PR #10004.
Wasmtime GitHub notifications bot (Jan 14 2025 at 01:23):fitzgen requested wasmtime-core-reviewers for a review on PR #10004.
Wasmtime GitHub notifications bot (Jan 14 2025 at 01:34):fitzgen updated PR #10004.
Wasmtime GitHub notifications bot (Jan 14 2025 at 05:24):cfallin submitted PR review:
This is great -- thanks a bunch for working this out!
My only thoughts are for a few more comments and one more test, otherwise LGTM.
Wasmtime GitHub notifications bot (Jan 14 2025 at 05:24):cfallin created PR review comment:
Can we add a comment here describing the rationale a bit more (for our future selves as much as others)?
Something like: "
side_effects_idempotentindicates that when we have more than one copy of this instruction, it is safe to merge (GVN) them. Traps have this characteristic: as long as we don't remove them, if we have already passed through (computed) one trap instruction on a given condition, it is safe to consider other traps on the same condition as redundant. In other words, a trap on value X is idempotent: once we observe its side effect (which will either terminate execution or do nothing), we don't need to observe it again."
Wasmtime GitHub notifications bot (Jan 14 2025 at 05:24):cfallin created PR review comment:
Can we also add a test with a
trapzandtrapnzon the same condition, to verify that (if we ever change the instruction encoding) we're disambiguating on the condition as well and not merging them? (i.e., arrange such that they would merge if they were both the same opcode)
Wasmtime GitHub notifications bot (Jan 14 2025 at 05:24):cfallin created PR review comment:
"Hit in GVN map but for instruction with zero results: may be a side-effect-only idempotent instruction like
trapnz/trapz. Nothing else to do here." perhaps?
Wasmtime GitHub notifications bot (Jan 14 2025 at 18:15):fitzgen updated PR #10004.
Wasmtime GitHub notifications bot (Jan 14 2025 at 18:15):fitzgen has enabled auto merge for PR #10004.
Wasmtime GitHub notifications bot (Jan 14 2025 at 18:18):fitzgen updated PR #10004.
Wasmtime GitHub notifications bot (Jan 14 2025 at 18:48):fitzgen merged PR #10004.
Last updated: Jan 24 2025 at 00:11 UTC