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 insertingbitcast
(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
andcanonicalise_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.
<!--
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.
-->
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
let any_non_canonical = values .iter() .any(|v| is_non_canonical_v128(builder.func.dfg.value_type(*v)));` `` ~~~
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
If we return
Option<SmallVec::<[ir::Value; 16]>>
here, we don't have to doSmallVec::<[ir::Value; 16]>::new()
logic in thecanonicalise_then_
yurydelendik created PR Review Comment:
let's split that into two functions:
canonicalise_then_br_z
andcanonicalise_then_br_nz
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
The reason for taking a
&mut SmallVec<[ir::Value; 16]>
rather than returning aOption<SmallVec::<[ir::Value; 16]>>
is to avoid copyingOption<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 mostvalues
are all non-vector.
alexcrichton submitted PR Review.
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 thecanonicalised
argument.
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
@alexcrichton Ah, that's neat. I'll do that.
abrown submitted PR Review.
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.
abrown submitted PR Review.
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
abrown submitted PR Review.
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 insertingbitcast
(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
andcanonicalise_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.
<!--
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.
-->
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 insertingbitcast
(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
andcanonicalise_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.
<!--
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.
-->
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 insertingbitcast
(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
andcanonicalise_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.
<!--
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.
-->
julian-seward1 requested yurydelendik for a review on PR #2303.
yurydelendik submitted PR Review.
julian-seward1 merged PR #2303.
Last updated: Dec 23 2024 at 12:05 UTC