Stream: git-wasmtime

Topic: wasmtime / PR #10154 pulley: Reimplement wasm loads/store...

Wasmtime GitHub notifications bot (Jan 29 2025 at 23:55):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #10154.

Wasmtime GitHub notifications bot (Jan 29 2025 at 23:55):

alexcrichton requested pchickey for a review on PR #10154.

Wasmtime GitHub notifications bot (Jan 29 2025 at 23:55):

alexcrichton requested wasmtime-core-reviewers for a review on PR #10154.

Wasmtime GitHub notifications bot (Jan 29 2025 at 23:55):

alexcrichton requested wasmtime-default-reviewers for a review on PR #10154.

Wasmtime GitHub notifications bot (Jan 29 2025 at 23:55):

alexcrichton requested fitzgen for a review on PR #10154.

Wasmtime GitHub notifications bot (Jan 29 2025 at 23:56):

alexcrichton commented on PR #10154:

I'll note that one thing I'm having difficulty adding a disas test for is something which generates a "g32bne" lowering. I can see the instructions getting emitted in spidermonkey.cwasm but I am having a difficult time reducing things down to something that can be added as an example. Most loads aren't "sinkable" which is why it's not used super often at least.

Wasmtime GitHub notifications bot (Jan 30 2025 at 02:06):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:meta", "isle", "pulley", "wasmtime:api"

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

• cfallin: isle
• fitzgen: isle, pulley

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 (Feb 04 2025 at 17:22):

fitzgen created PR review comment:

All the instruction deltas in this file are equal or worse compared to before -- is that expected?

Wasmtime GitHub notifications bot (Feb 04 2025 at 17:22):

fitzgen created PR review comment:

Can we put this in the common prelude, rather than duplicating it in both the opt and lower preludes?

Wasmtime GitHub notifications bot (Feb 04 2025 at 17:22):

fitzgen submitted PR review:

LGTM but some questions that should be resolved below.

Wasmtime GitHub notifications bot (Feb 04 2025 at 17:22):

fitzgen created PR review comment:

Might as well use the helper if you're gonna define it:

        self.is_pulley()

Wasmtime GitHub notifications bot (Feb 04 2025 at 17:23):

fitzgen commented on PR #10154:

And sorry for the delay on this review, I forgot about it for a little bit

Wasmtime GitHub notifications bot (Feb 06 2025 at 14:18):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 06 2025 at 14:18):

alexcrichton created PR review comment:

It was originally, but the problem is that they're slightly different where the helpers for egraphs have a ty variable and the helpers for backends don't. I'm not sure how to otherwise unify those two myself

Wasmtime GitHub notifications bot (Feb 06 2025 at 14:19):

alexcrichton updated PR #10154.

Wasmtime GitHub notifications bot (Feb 06 2025 at 15:13):

alexcrichton updated PR #10154.

Wasmtime GitHub notifications bot (Feb 06 2025 at 15:29):

alexcrichton created PR review comment:

That's a good point, and one where I mostly shrugged this off but shouldn't have! I focused a bit more on this and improved a few cases in https://github.com/bytecodealliance/wasmtime/pull/10154/commits/63e2c5a92a3a209b466a8e3a7627bcb9cd3ef159

The remaining ones that got worse are:

• offset_just_bad_v2 - looking into this it looks like optimizations shuffle constants around enough that the output CLIF is quite different from normal loads/stores, so I think this is something we'll basically just take a hit on for now and improve later if necessary
• maybe_inbounds_v2 - this doesn't codegen well because egraphs can't optimize away the uadd_overflow_trap-with-iconst-inputs that the frontend generates. Another possible fix is to optimize the frontend to skip uadd_overflow_trap in this case (constant address).

Wasmtime GitHub notifications bot (Feb 06 2025 at 15:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 06 2025 at 15:30):

alexcrichton updated PR #10154.

Wasmtime GitHub notifications bot (Feb 06 2025 at 16:53):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 06 2025 at 16:53):

cfallin created PR review comment:

FWIW, we want to add the type parameters in the lowering prelude's versions of the bindings eventually, for exactly this reason -- the only reason they aren't there is that lowering rules were written first and we didn't realize we would need them on the constructor side. @mmcloughlin apparently has a private branch where this change has been made -- Michael, would you be willing to think about upstreaming this?

Wasmtime GitHub notifications bot (Feb 06 2025 at 20:10):

mmcloughlin submitted PR review.

Wasmtime GitHub notifications bot (Feb 06 2025 at 20:10):

mmcloughlin created PR review comment:

Yes, I think I could upstream it. Our fork has diverged a bit so it's not a clean patch, but thankfully I did keep the Python script I used to do most of the fixups and hopefully that procedure would work again.

A couple of things to note about the change in our fork:

• The ty parameter is not always present on instruction extractors. In the CDSL code generator, a ty parameter added only when the instruction is statically known to always have at least one result. In that case, ty is the type of the first result value.
• The existing code uses has_type extractor in cases that would now be more naturally be done with the provided ty argument. Replacing has_type uses with the ty argument is hard/impossible to do automatically. This is fine as long as you're okay with migrating away from has_type (where possible) more slowly over time.
  

Wasmtime GitHub notifications bot (Feb 06 2025 at 20:37):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 06 2025 at 20:37):

cfallin created PR review comment:

The ty parameter is not always present on instruction extractors. In the CDSL code generator, a ty parameter added only when the instruction is statically known to always have at least one result. In that case, ty is the type of the first result value.

I suppose we could use Types::INVALID for no-result instructions on the extractor side. (On the constructor side we currently don't generate ctors for side-effecting insts, which includes all no-result insts; but when we do, we could require Types::INVALID to be passed in, asserting otherwise.)

The existing code uses has_type extractor in cases that would now be more naturally be done with the provided ty argument. Replacing has_type uses with the ty argument is hard/impossible to do automatically. This is fine as long as you're okay with migrating away from has_type (where possible) more slowly over time.

Yeah, a gradual migration seems totally fine here.

Wasmtime GitHub notifications bot (Feb 10 2025 at 18:26):

alexcrichton merged PR #10154.

Last updated: Feb 28 2025 at 02:27 UTC