wasmtime / PR #2303 wasm->CLIF translation: consistently ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2303 wasm->CLIF translation: consistently ...

Wasmtime GitHub notifications bot (Oct 20 2020 at 14:01):

julian-seward1 opened PR #2303 from improve-vec-bitcasting to main:

…ck formal parameters.

In the current translation of wasm (128-bit) SIMD into CLIF, we work around differences in the
type system models of wasm vs CLIF by inserting bitcast (no-op casts) CLIF instructions before
more or less every use of a SIMD value. Unfortunately this was not being done consistently and
even small examples with a single if-then-else diamond that produces a SIMD value, could cause a
verification failure downstream. In this case, the jump out of the "else" block needed a
bitcast, but didn't have one.

This patch wraps creation of CLIF jumps and conditional branches up into a pair of functions,
canonicalise_then_jump and canonicalise_then_br_z_or_nz, and uses them consistently. They
first cast the relevant block formal parameters, then generate the relevant kind of branch/jump.
Hence, provided they are also used consistently in future to generate branches/jumps in this
file, we are protected against such failures.

The patch also adds a large(ish) comment at the top explaining this in more detail.

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:00):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:00):

yurydelendik created PR Review Comment:

let any_non_canonical = values
  .iter()
  .any(|v| is_non_canonical_v128(builder.func.dfg.value_type(*v)));`
``
~~~

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:00):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:00):

yurydelendik created PR Review Comment:

If we return Option<SmallVec::<[ir::Value; 16]>> here, we don't have to do SmallVec::<[ir::Value; 16]>::new() logic in the canonicalise_then_

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:00):

yurydelendik created PR Review Comment:

let's split that into two functions: canonicalise_then_br_z and canonicalise_then_br_nz

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:06):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:06):

julian-seward1 created PR Review Comment:

The reason for taking a &mut SmallVec<[ir::Value; 16]> rather than returning a Option<SmallVec::<[ir::Value; 16]>> is to avoid copying Option<SmallVec::<[ir::Value; 16]>> in the return at each call. In the way I had it, canonicalise_v128_values never has to copy the vector (at return), because the caller owns it, and it very rarely even has to put anything in the vector, because most values are all non-vector.

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:12):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:12):

alexcrichton created PR Review Comment:

One simplification that could also make this more convenient to call would be the signature:
fn canonicalize_v128_values<'a>(
    canonicalized: &'a mut SmallVec<[ir::Value; 16]>,
    builder: ..,
    values: &'a [ir::Value],
) -> &'a [ir::Value] {
    // ...
}
which either returns the original values argument or returns a slice of the canonicalised argument.

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:23):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 20 2020 at 17:23):

julian-seward1 created PR Review Comment:

@alexcrichton Ah, that's neat. I'll do that.

Wasmtime GitHub notifications bot (Oct 20 2020 at 18:02):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 20 2020 at 18:02):

abrown created PR Review Comment:

Can you link to https://github.com/bytecodealliance/wasmtime/issues/1147 and optionally add these comments there, or vice-versa? In that issue I discuss other alternatives as well.

Wasmtime GitHub notifications bot (Oct 20 2020 at 18:04):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 20 2020 at 18:04):

abrown created PR Review Comment:

Also, you may want to reference prior work:

I already tried the single v128 type approach: https://github.com/bytecodealliance/cranelift/pull/1251

I half-heartedly tried just relaxing the verification: https://github.com/bytecodealliance/cranelift/pull/1236

Wasmtime GitHub notifications bot (Oct 20 2020 at 18:09):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 21 2020 at 09:20):

julian-seward1 updated PR #2303 from improve-vec-bitcasting to main:

…ck formal parameters.

In the current translation of wasm (128-bit) SIMD into CLIF, we work around differences in the
type system models of wasm vs CLIF by inserting bitcast (no-op casts) CLIF instructions before
more or less every use of a SIMD value. Unfortunately this was not being done consistently and
even small examples with a single if-then-else diamond that produces a SIMD value, could cause a
verification failure downstream. In this case, the jump out of the "else" block needed a
bitcast, but didn't have one.

This patch wraps creation of CLIF jumps and conditional branches up into a pair of functions,
canonicalise_then_jump and canonicalise_then_br_z_or_nz, and uses them consistently. They
first cast the relevant block formal parameters, then generate the relevant kind of branch/jump.
Hence, provided they are also used consistently in future to generate branches/jumps in this
file, we are protected against such failures.

The patch also adds a large(ish) comment at the top explaining this in more detail.

Wasmtime GitHub notifications bot (Oct 21 2020 at 09:25):

julian-seward1 updated PR #2303 from improve-vec-bitcasting to main:

…ck formal parameters.

In the current translation of wasm (128-bit) SIMD into CLIF, we work around differences in the
type system models of wasm vs CLIF by inserting bitcast (no-op casts) CLIF instructions before
more or less every use of a SIMD value. Unfortunately this was not being done consistently and
even small examples with a single if-then-else diamond that produces a SIMD value, could cause a
verification failure downstream. In this case, the jump out of the "else" block needed a
bitcast, but didn't have one.

This patch wraps creation of CLIF jumps and conditional branches up into a pair of functions,
canonicalise_then_jump and canonicalise_then_br_z_or_nz, and uses them consistently. They
first cast the relevant block formal parameters, then generate the relevant kind of branch/jump.
Hence, provided they are also used consistently in future to generate branches/jumps in this
file, we are protected against such failures.

The patch also adds a large(ish) comment at the top explaining this in more detail.

Wasmtime GitHub notifications bot (Oct 21 2020 at 09:28):

julian-seward1 updated PR #2303 from improve-vec-bitcasting to main:

…ck formal parameters.

In the current translation of wasm (128-bit) SIMD into CLIF, we work around differences in the
type system models of wasm vs CLIF by inserting bitcast (no-op casts) CLIF instructions before
more or less every use of a SIMD value. Unfortunately this was not being done consistently and
even small examples with a single if-then-else diamond that produces a SIMD value, could cause a
verification failure downstream. In this case, the jump out of the "else" block needed a
bitcast, but didn't have one.

This patch wraps creation of CLIF jumps and conditional branches up into a pair of functions,
canonicalise_then_jump and canonicalise_then_br_z_or_nz, and uses them consistently. They
first cast the relevant block formal parameters, then generate the relevant kind of branch/jump.
Hence, provided they are also used consistently in future to generate branches/jumps in this
file, we are protected against such failures.

The patch also adds a large(ish) comment at the top explaining this in more detail.

Wasmtime GitHub notifications bot (Oct 21 2020 at 10:32):

julian-seward1 requested yurydelendik for a review on PR #2303.

Wasmtime GitHub notifications bot (Oct 21 2020 at 14:48):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 21 2020 at 15:43):

julian-seward1 merged PR #2303.

Last updated: Oct 23 2024 at 20:03 UTC