Stream: git-wasmtime

Topic: wasmtime / PR #10524 Cranelift: simplify some side-effect...

Wasmtime GitHub notifications bot (Apr 04 2025 at 23:31):

fitzgen opened PR #10524 from fitzgen:simplify-skeleton to bytecodealliance:main:

This commit adds a new top-level ISLE entrypoint specifically for instructions in the side-effectful skeleton: simplify_skeleton. While these rewrites are processed during the egraph pass, values from skeleton instructions still do not get inserted into the egraph. Indeed, simplify_skeleton operates on instructions rather than values because we do not represent side effects as values; values do not have side effects in CLIF, instructions do. Therefore, rather than doing a whole dynamic-programming style extraction of the best candidate simplification like we do with the egraph, we take an eager and greedy approach.

Furthermore, simplify_skeleton is limited only to skeleton instructions that do not involve control-flow or terminators right now. This is because changing the control-flow graph can change whether a use is dominated by a def or not, and we do not currently have the machinery to track and fix up invalidated uses. Addressing this is left for future commits.

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Apr 04 2025 at 23:31):

fitzgen requested cfallin for a review on PR #10524.

Wasmtime GitHub notifications bot (Apr 04 2025 at 23:31):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #10524.

Wasmtime GitHub notifications bot (Apr 04 2025 at 23:31):

fitzgen requested pchickey for a review on PR #10524.

Wasmtime GitHub notifications bot (Apr 04 2025 at 23:31):

fitzgen requested wasmtime-core-reviewers for a review on PR #10524.

Wasmtime GitHub notifications bot (Apr 04 2025 at 23:50):

fitzgen commented on PR #10524:

FWIW, I just did a sightglass run, and this had no statistically significant effect on any of our default benchmarks. If you look at the disas test expectation updates however, you’ll see that it is a big boon for gc codegen.

Wasmtime GitHub notifications bot (Apr 04 2025 at 23:54):

fitzgen updated PR #10524.

Wasmtime GitHub notifications bot (Apr 05 2025 at 02:13):

github-actions[bot] commented on PR #10524:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "cranelift:meta", "isle"

Thus the following users have been cc'd because of the following labels:

• cfallin: isle
• fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Apr 08 2025 at 00:06):

cfallin submitted PR review:

Looks great -- very cool to see the GC tests clean up with the new opts in particular!

I think I mentioned it last week but just to write it down: I suspect a good way to lift some of the CFG-mutation limitations around traps would be to make trap a non-terminator, and to add unreachable as a terminator. This would allow us to turn insts that we can prove always trap into trap (e.g. the sdiv case mentioned below, as well as trapnz-of-1 and trapz-of-0). The branch-folding cases are a bit harder; ideally we do actually skip traversing the newly-unreachable blocks, perhaps by tombstoning them with a flag set by the branch simplification. Would you mind creating a followup issue for this?

Wasmtime GitHub notifications bot (Apr 08 2025 at 00:06):

cfallin created PR review comment:

Idle thought: we have the "temporarily take a reused thing then clear it" pattern in several places -- would it be possible to factor this out into a macro, with pluggable bits for the vec type and parent type and field name?

Wasmtime GitHub notifications bot (Apr 08 2025 at 00:06):

cfallin created PR review comment:

Can we add a test for INT_MIN / -1 and show that it remains un-cprop'd?

(It would be even neater if it could change into an unconditional trap but I know that's a limitation at the moment)

Wasmtime GitHub notifications bot (Apr 08 2025 at 16:03):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 08 2025 at 16:03):

fitzgen created PR review comment:

Sure, I can do this in a follow up

Wasmtime GitHub notifications bot (Apr 08 2025 at 16:23):

fitzgen commented on PR #10524:

I think I mentioned it last week but just to write it down: I suspect a good way to lift some of the CFG-mutation limitations around traps would be to make trap a non-terminator, and to add unreachable as a terminator.

Yes, we could even avoid a new unreachable instruction if we wanted by adding a terminator: bool to trap, if we wanted, since we should have the bytes available in the InstructionData.

Or if we do want two different instructions, it is probably easier to make the new instruction the non-terminator, so that existing uses of trap don't need updating.

I had also floated the idea of trap returning a variable number of values, that we could use as dummy values in now-unreachable uses, just as a convenience and to make the branch-folding a little simpler to deal with (rather than requiring tracking reachability perfectly). This could be an intermediate step since it would allow us to avoid perfectly tracking and fixing up unreachable blocks, but keeping dominating defs for any now-unreachable blocks' uses.

The branch-folding cases are a bit harder; ideally we do actually skip traversing the newly-unreachable blocks, perhaps by tombstoning them with a flag set by the branch simplification. Would you mind creating a followup issue for this?

We also would need to differentiate between blocks that do vs don't become unreachable, which is a further little wrinkle.

But yeah, happy to write up an issue.

Wasmtime GitHub notifications bot (Apr 08 2025 at 17:31):

fitzgen updated PR #10524.

Wasmtime GitHub notifications bot (Apr 08 2025 at 17:38):

fitzgen updated PR #10524.

Wasmtime GitHub notifications bot (Apr 08 2025 at 17:38):

fitzgen has enabled auto merge for PR #10524.

Wasmtime GitHub notifications bot (Apr 08 2025 at 18:18):

fitzgen merged PR #10524.

Last updated: Dec 06 2025 at 06:05 UTC