Stream: git-wasmtime

Topic: wasmtime / PR #9759 pulley: Support big-endian targets

Wasmtime GitHub notifications bot (Dec 06 2024 at 20:55):

alexcrichton requested fitzgen for a review on PR #9759.

Wasmtime GitHub notifications bot (Dec 06 2024 at 20:55):

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

Wasmtime GitHub notifications bot (Dec 06 2024 at 20:55):

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

Wasmtime GitHub notifications bot (Dec 06 2024 at 20:55):

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

Wasmtime GitHub notifications bot (Dec 06 2024 at 20:55):

alexcrichton opened PR #9759 from alexcrichton:pulley-big-endian to bytecodealliance:main:

This commit fixes the Pulley interpreter and Cranelift backend to properly support big-endian targets. The problem beforehand was that loads/stores in Pulley were defined as the native endianness which means that big and little-endian platforms differed. Additionally the previously set of understood pulley targets were implicitly always little-endian which was not appropriate for a big-endian host where JIT data structures are stored in big-endian format.

This commit fixes all of these issues with a few changes:

• Pulley loads/stores are always little-endian now.
• Pulley now has bswap32/bswap64 instructions.
• Wasmtime/Cranelift now understand big-endian pulley targets (e.g. pulley32be).
• CLIF translation of loads/stores now properly handles the endianness flags on MemFlags.

In the future if necessary we could natively add a macro-op for big-endian loads/stores to Pulley but for now it's more minimal to just have bswap32 and bswap64.

The result of this commit is that Pulley tests are now run and executed on s390x like all other platforms.

<!--
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 (Dec 06 2024 at 20:56):

alexcrichton commented on PR #9759:

While this doesn't code-wise depend on https://github.com/bytecodealliance/wasmtime/pull/9758 it test-wise depends on that PR as otherwise unwinding doesn't actually work on s390x. With both PRs, however, tests pass for Pulley.

Wasmtime GitHub notifications bot (Dec 06 2024 at 21:17):

alexcrichton updated PR #9759.

Wasmtime GitHub notifications bot (Dec 06 2024 at 22:21):

fitzgen submitted PR review:

r=me with the two below suggestions

Wasmtime GitHub notifications bot (Dec 06 2024 at 22:21):

fitzgen created PR review comment:

Can we instead have a pure constructor like

(type Endian (enum Little Big))

(decl endian (MemFlags) Endian)
(extern pure constructor endian endian)

Wasmtime GitHub notifications bot (Dec 06 2024 at 22:21):

fitzgen created PR review comment:

... which we can then use like

(decl xswap_if_be (XReg Type MemFlags) XReg)
(rule 0 (xswap_if_be val _ty flags)
      (if-let (endian flags) (Endian.Little))
      val)
(rule 1 (xswap_if_be val $I32 flags)
      (if-let (endian flags) (Endian.Big))
      (pulley_bswap32 val))
(rule 1 (xswap_if_be val $I64 flags)
      (if-let (endian flags) (Endian.Big))
      (pulley_bswap64 val))

and I think we could remove the explicit priorities at that point because isle's overlap checking would be able to see that the rules do not overlap, which would also result in better isle-generated rust code.

Wasmtime GitHub notifications bot (Dec 06 2024 at 22:21):

fitzgen created PR review comment:

Want to move these to extended ops? I doubt they will be common.

Wasmtime GitHub notifications bot (Dec 06 2024 at 22:37):

alexcrichton updated PR #9759.

Wasmtime GitHub notifications bot (Dec 06 2024 at 22:37):

alexcrichton has enabled auto merge for PR #9759.

Wasmtime GitHub notifications bot (Dec 06 2024 at 22:40):

alexcrichton updated PR #9759.

Wasmtime GitHub notifications bot (Dec 06 2024 at 23:14):

alexcrichton merged PR #9759.

Last updated: Dec 23 2024 at 12:05 UTC