Stream: git-wasmtime

Topic: wasmtime / PR #3202 Fix validating wasm stores of boolean...

Wasmtime GitHub notifications bot (Aug 18 2021 at 15:49):

alexcrichton opened PR #3202 from fix-simd-store to main:

Previously cranelift's wasm code generator would emit a raw store
instruction for all wasm types, regardless of what the cranelift operand
type was. Cranelift's store instruction, however, isn't valid for
boolean vector types. This commit fixes this issue by inserting a
bitcast specifically for the store instruction if a boolean vector type
is being stored, continuing to avoid the bitcast for all other vector types.

Closes #3099

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 18 2021 at 15:51):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2021 at 16:09):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2021 at 16:09):

bjorn3 created PR review comment:

Should we instead define that boolean vectors can be stored using store directly and are stored as all-zero or all-one lanes?

Wasmtime GitHub notifications bot (Aug 18 2021 at 16:22):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2021 at 16:22):

abrown created PR review comment:

That is a good approach but I don't mind what @alexcrichton tried here because:

• vector translation already involves a lot of bitcasting so this doesn't make the problem worse; if we want to solve the bitcasting we should just solve all of and close #1147
• changing load and store to allow boolean vectors but not boolean scalars seems like it might cause errors somewhere; this fix would immediately fix a fuzz bug
• I wouldn't mind settling the general issue about loads and stores with booleans once for all: I've seen mention recently of setting a memory representation for boolean scalars... If we settle it once for all (do we have an issue for this?) and document it then it might make sense to avoid the bitcast (if that's what is decided).

Wasmtime GitHub notifications bot (Aug 18 2021 at 16:22):

abrown edited PR review comment.

Wasmtime GitHub notifications bot (Aug 18 2021 at 17:04):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2021 at 17:04):

cfallin created PR review comment:

I just created #3205 for this. Agreed that for now, the immediate fuzzbug fix is fine.

Wasmtime GitHub notifications bot (Aug 18 2021 at 17:16):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2021 at 17:16):

alexcrichton created PR review comment:

Ok I'm gonna go ahead and merge this fix for now and defer to #3205 for future updates.

Wasmtime GitHub notifications bot (Aug 18 2021 at 17:16):

alexcrichton merged PR #3202.

Last updated: Dec 23 2024 at 12:05 UTC