Stream: git-wasmtime

Topic: wasmtime / PR #9850 cranelift: 32bit div_s, rem_u, rem_s ...

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 12:14):

MarinPostma opened PR #9850 from MarinPostma:i32-idiv-aarch64 to bytecodealliance:main:

followup to #9798 where I did the ground work for 32bit division, this PR extends 32bits optimizations to rem_u, rem_s and div_s.

This should close #9766.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 12:14):

MarinPostma requested wasmtime-compiler-reviewers for a review on PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 12:14):

MarinPostma requested cfallin for a review on PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 13:38):

MarinPostma updated PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 15:25):

MarinPostma updated PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 16:55):

MarinPostma updated PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 16:58):

MarinPostma updated PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:07):

MarinPostma updated PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:32):

MarinPostma updated PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:33):

MarinPostma updated PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:38):

cfallin submitted PR review:

Looks good, thanks a bunch! Very nice to see the test expectations get shorter. A few thoughts below but nothing major.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:38):

cfallin created PR review comment:

Can we say (fits_in_32 ty) rather than ty here, and make this the higher-priority case (so 64-bit is the fallback)? That seems a little cleaner to me than the implicit "everything not I64 is smaller than 64 bits" here (and less likely to break if we try to do other things like support I128 more fully in the future).

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:38):

cfallin created PR review comment:

Can we merge the SDiv and UDiv cases now?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:38):

cfallin created PR review comment:

can we make this a match (with Sign and Zero cases)?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:38):

cfallin created PR review comment:

cc @avanhatt @mmcloughlin -- maybe the first instance of active work on the aarch64 backend that needs to update a spec. I definitely don't think we should block this PR on it (so don't worry about this, @MarinPostma!) but it's worth thinking what our short and medium term approaches will be to this since we're upstreamed but don't have a nice CI-integrated workflow yet -- should we keep a queue of such TODOs somewhere?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:38):

cfallin created PR review comment:

s/entension/extension/
s/perform/performs/

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:38):

cfallin created PR review comment:

Likewise here, and we can pull the if bits < 64 in as a guard on one of the match arms as well.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:38):

cfallin created PR review comment:

Debugging code left in?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:44):

MarinPostma commented on PR #9850:

hey @cfallin, fixign a bunch of stuff, that's why I put it in draft, but I'll include your review, as soon I manage to fix the tests :)

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:46):

MarinPostma submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:46):

MarinPostma created PR review comment:

about that: I meant to restore them at some point, but I don't know how to run the verification

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 18:48):

MarinPostma edited a comment on PR #9850:

hey @cfallin, fixing a bunch of stuff, that's why I put it in draft, but I'll include your review, as soon I manage to fix the tests :)

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 19:01):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 19:01):

cfallin created PR review comment:

Yep, the integration with the normal dev workflow is still very much an open question; we could have you ramp up on that but I don't think it's at the point that we want to require that of everyone yet.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 18 2024 at 20:45):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

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

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

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2024 at 12:29):

MarinPostma updated PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2024 at 13:06):

MarinPostma has marked PR #9850 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2024 at 13:06):

MarinPostma requested cfallin for a review on PR #9850.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2024 at 19:20):

cfallin submitted PR review:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Dec 19 2024 at 19:41):

cfallin merged PR #9850.

Last updated: Dec 23 2024 at 12:05 UTC