wasmtime / PR #10109 Winch: `v128` logical ops for x64 · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #10109 Winch: `v128` logical ops for x64

Wasmtime GitHub notifications bot (Jan 24 2025 at 22:01):

MarinPostma edited PR #10109.

Wasmtime GitHub notifications bot (Jan 24 2025 at 22:07):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 24 2025 at 22:09):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 24 2025 at 22:09):

MarinPostma edited PR #10109:

implement v128 simd logical operations:

v128.not

v128.and

v128.andnot

v128.or

v128.xor

https://github.com/bytecodealliance/wasmtime/issues/8093

Wasmtime GitHub notifications bot (Jan 24 2025 at 22:37):

alexcrichton commented on PR #10109:

Moving my review over to @saulecabrera who knows this better than I

Wasmtime GitHub notifications bot (Jan 24 2025 at 22:37):

alexcrichton requested saulecabrera for a review on PR #10109.

Wasmtime GitHub notifications bot (Jan 24 2025 at 23:44):

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

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "winch"

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

saulecabrera: winch

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 (Jan 25 2025 at 14:04):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 25 2025 at 14:05):

MarinPostma edited PR #10109:

implement v128 simd logical operations:

v128.not

v128.and

v128.andnot

v128.or

v128.xor

v128.bitselect

https://github.com/bytecodealliance/wasmtime/issues/8093

Wasmtime GitHub notifications bot (Jan 25 2025 at 15:19):

MarinPostma edited PR #10109:

implement v128 simd logical operations:

v128.not

v128.and

v128.andnot

v128.or

v128.xor

v128.bitselect

v128.any_true

https://github.com/bytecodealliance/wasmtime/issues/8093

Wasmtime GitHub notifications bot (Jan 25 2025 at 15:19):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 25 2025 at 16:20):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 25 2025 at 16:21):

MarinPostma edited PR #10109:

implement v128 simd logical operations:

v128.not

v128.and

v128.andnot

v128.or

v128.xor

v128.bitselect

v128.any_true

v128.load8_lane

v128.load16_lane

v128.load32_lane

v128.load64_lane

https://github.com/bytecodealliance/wasmtime/issues/8093

Wasmtime GitHub notifications bot (Jan 25 2025 at 17:41):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 25 2025 at 17:42):

MarinPostma edited PR #10109:

implement v128 simd logical operations:

v128.not

v128.and

v128.andnot

v128.or

v128.xor

v128.bitselect

v128.any_true

v128.load8_lane

v128.load16_lane

v128.load32_lane

v128.load64_lane

v128.store8_lane

v128.store16_lane

v128.store32_lane

v128.store64_lane

https://github.com/bytecodealliance/wasmtime/issues/8093

Wasmtime GitHub notifications bot (Jan 27 2025 at 11:48):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 27 2025 at 13:33):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jan 27 2025 at 13:33):

saulecabrera created PR review comment:

Instead of introducing new load/stored methods, I'd suggest calling wasm_load and wasm_store. This approach would probably require modifying the existing LoadKind definition in case the current definition can't represent the semantics of lane loading/storing. That's the approach we've followed for other vector loads (e.g., load_splat)

1 main reason:

Wasm loads/stores are critical, from the sandboxing perspective, and we're trying to keep the implementation as tight as possible, reusing the existing methods makes auditing them and maintaining them much easier.

Wasmtime GitHub notifications bot (Jan 27 2025 at 13:33):

saulecabrera created PR review comment:

A small remark on naming: to be consistent with the rest of the naming in the MacroAssembler for type-specific instructions, perhaps we could consider prefixing all these methods with v128_? e.g., v128_or, similar to float operations, e.g., float_abs

Wasmtime GitHub notifications bot (Jan 27 2025 at 13:33):

saulecabrera created PR review comment:

We need to check that the host/target supports AVX for all of these methods:
if !self.flags.has_avx() {
  bail!(CodeGenError::UnimplementedForNoAvx);
}

Wasmtime GitHub notifications bot (Jan 27 2025 at 20:23):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 27 2025 at 20:26):

MarinPostma commented on PR #10109:

@saulecabrera it should be good now. I have take the opportunity to tighten store/loads even more regarding atomics operation. Having the MemOpKind as a separate arg meant that we could declare many opeartion as atomics, but that would have no effect.

Wasmtime GitHub notifications bot (Jan 27 2025 at 21:53):

MarinPostma edited a comment on PR #10109:

@saulecabrera it should be good now. I have taken the opportunity to tighten store/loads even more regarding atomics operation. Having the MemOpKind as a separate arg meant that we could declare many opeartion as atomics, but that would have no effect.

Wasmtime GitHub notifications bot (Jan 28 2025 at 12:42):

saulecabrera submitted PR review:

LGTM, thanks!

Wasmtime GitHub notifications bot (Jan 28 2025 at 12:43):

saulecabrera commented on PR #10109:

There's a conflict, once resolved, we can land this one.

Wasmtime GitHub notifications bot (Jan 28 2025 at 15:43):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 28 2025 at 15:44):

MarinPostma commented on PR #10109:

done

Wasmtime GitHub notifications bot (Jan 28 2025 at 18:48):

MarinPostma commented on PR #10109:

looks like a spurious failure? @saulecabrera

Wasmtime GitHub notifications bot (Jan 28 2025 at 19:05):

alexcrichton commented on PR #10109:

it's a bit hidden (alas) but I think that failure was non-spurious: https://github.com/bytecodealliance/wasmtime/actions/runs/13016537423/job/36307106911

Wasmtime GitHub notifications bot (Jan 28 2025 at 19:13):

saulecabrera commented on PR #10109:

Yeah, doesn't seem to be spurious. You need to update the spec tests here https://github.com/bytecodealliance/wasmtime/blob/main/crates/wast-util/src/lib.rs#L492 to include the ones that you enabled, so that it's marked as expected that these tests should fail on architectures that don't support AVX+. I believe this is mostly for for darwin x86_64, which runs via Rosetta and therefore there's no AVX support.

Wasmtime GitHub notifications bot (Jan 28 2025 at 21:58):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 28 2025 at 22:01):

MarinPostma commented on PR #10109:

I think I have fixed it, but I don't have a mac to try it right now. Thanks!

Wasmtime GitHub notifications bot (Jan 29 2025 at 01:34):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jan 29 2025 at 01:34):

saulecabrera created PR review comment:

The failures that CI are related to the vpinsr*instruction, which are emitted here. We need to add self.ensure_has_avx()? to get CI green.

Wasmtime GitHub notifications bot (Jan 29 2025 at 01:35):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Jan 29 2025 at 09:47):

MarinPostma updated PR #10109.

Wasmtime GitHub notifications bot (Jan 29 2025 at 11:17):

MarinPostma submitted PR review.

Wasmtime GitHub notifications bot (Jan 29 2025 at 11:17):

MarinPostma created PR review comment:

I setup the action on my fork, all tests pass now. worry for the back and forth :sweat:

Wasmtime GitHub notifications bot (Jan 29 2025 at 12:16):

saulecabrera merged PR #10109.

Last updated: Feb 28 2025 at 03:10 UTC