Stream: git-wasmtime

Topic: wasmtime / PR #2783 Add support for x64 packed promote low

Wasmtime GitHub notifications bot (Mar 26 2021 at 22:46):

jlb6740 opened PR #2783 from implement_64_packed_promotelow_demote to main:

<!--

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 (Mar 27 2021 at 00:18):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (Mar 27 2021 at 00:19):

jlb6740 requested abrown for a review on PR #2783.

Wasmtime GitHub notifications bot (Mar 27 2021 at 00:20):

jlb6740 requested cfallin and abrown for a review on PR #2783.

Wasmtime GitHub notifications bot (Mar 27 2021 at 02:37):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Mar 27 2021 at 02:37):

abrown created PR Review Comment:

For this one, I think it does make sense to keep the _low since it indicates which lanes get promoted, but I would drop the v since the _low already implies lanes.

Wasmtime GitHub notifications bot (Mar 27 2021 at 02:37):

abrown created PR Review Comment:

I would suggest trying to fit this under fdemote: the semantics are very close and we could document on fdemote that, when used on vector types, it's conversions are placed in the lowest lanes. E.g., when demoting lanes 0 and 1 of an f64x2, the results are placed in lanes 0 and 1 of the f32x4). We will also want to document that lanes 2 and 3 are zeroed out.

Wasmtime GitHub notifications bot (Mar 27 2021 at 02:37):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Mar 27 2021 at 02:37):

abrown created PR Review Comment:

This allows fpromote.f64x2 and fpromote_low.f64x2 to compile to the same x64 instruction, which I don't think is intended. Perhaps assert that the vectors have op == Opcode::FpromoteLow and the scalars have op == Opcode::Fpromote. (This comment also applies below if you don't do the "fvdemote -> fdemote" renaming).

Wasmtime GitHub notifications bot (May 13 2021 at 20:28):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (May 21 2021 at 04:43):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (May 26 2021 at 19:16):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (May 26 2021 at 20:18):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (May 26 2021 at 20:36):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 26 2021 at 20:36):

jlb6740 created PR review comment:

Yes, I don't disagree about combining fdemote and fvdemote feeling like the natural thing to do. In fact I tried that first as but doing that failed due to the constraint requiring the lane sizes remain the same. Trying to support both both f32.demote and f32x4.demote_f64x2_zero with fdemote would require us to make this instruction definition less specific. In that case comments about zeroing lanes would not apply to the general instruction here as it only applies when lowering f32x4.demote_f64x2_zero? If not combined then certainly adding that zeroing part makes sense. Even though the language is there and seemly necessary in instruction.rs, frankly I don't really see the concept of lanes with the definition of f32.demote. But that's a different subject. What do you think about the implication of making the fdemote instruction comments less specific if we are trying to cover the two different lowerings?

Wasmtime GitHub notifications bot (May 26 2021 at 20:36):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 26 2021 at 21:02):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 26 2021 at 21:02):

abrown created PR review comment:

        Considering only the lower half of the register, the low lanes in `x` are interpreted as
        single precision floats that are then converted to double precision floats.

Wasmtime GitHub notifications bot (May 26 2021 at 21:02):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 26 2021 at 21:02):

abrown created PR review comment:

I think we still need to think about this comment...

Wasmtime GitHub notifications bot (May 26 2021 at 21:02):

abrown created PR review comment:

I also see .constraints(vec![WiderOrEq(Float.clone(), FloatTo.clone())]), used in a few places, which might be a more generic approach.

Wasmtime GitHub notifications bot (May 26 2021 at 21:02):

abrown created PR review comment:

    let FxN = &TypeVar::new(
        "FxN",
        "A vector floating point number",
        TypeSetBuilder::new()
            .floats(Interval::All)
            .simd_lanes(Interval::All)
            .includes_scalars(false)
            .build(),
    );
    let x = &Operand::new("x", FxN);
    let a = &Operand::new("a", FxN.merge_lanes());

I think if we want to be more precise with the types here we should use a "vector float" type (e.g. build a new FxN) and tell the type system using .merge_lanes() that the result will have the same number of bits, just half the number of lanes, like we do in the documentation.

Wasmtime GitHub notifications bot (Jun 02 2021 at 18:36):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (Jun 02 2021 at 19:12):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (Jun 02 2021 at 19:16):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (Jun 02 2021 at 20:05):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jun 02 2021 at 20:05):

jlb6740 created PR review comment:

Reverted back to having a separate lowering for fvdemote after some offline discussion with @abrown. The impact of adding a constraint was not clear. For example, I thought if adding the constraint:

.constraints(vec![WiderOrEq(Float.clone(), FloatTo.clone())]),

this meant that x needed to be wider or equal to a. When I did this the spec test passed as expected. However, when I reversed the constraint to say

.constraints(vec![WiderOrEq(FloatTo.clone(), Float.clone())]),

This also passed. For this reason I just left it out which is what the original patch submission did (as written in the notes).

Wasmtime GitHub notifications bot (Jun 02 2021 at 20:07):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jun 02 2021 at 20:07):

jlb6740 created PR review comment:

Note the original patch used fvpromote_low. This was to be consistent with fvdemote. However there was a desire to rename fvpromote_low to fpromote_low and also git rid of fvdemote (combine with fdemote). Now that fvdemote is back, the naming is inconsistent here but that may be OK.

Wasmtime GitHub notifications bot (Jun 02 2021 at 20:07):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Jun 03 2021 at 22:15):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jun 03 2021 at 22:15):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jun 03 2021 at 22:15):

abrown created PR review comment:

Same type comments apply here.

Wasmtime GitHub notifications bot (Jun 03 2021 at 22:15):

abrown created PR review comment:

            "fvpromote_low",

Wasmtime GitHub notifications bot (Jun 03 2021 at 22:15):

abrown created PR review comment:

We need to make sure that we constrain this so that the following is not possible:

v1 = fconst.f64x2 ...
v2 = fvdemote.f32 v1

Right now this looks possible, but we shouldn't be able to demote from a vector to a scalar (doesn't make sense). If we add the WiderOrEq(x, a) constraint, though, the types allowed should be, e.g., f64x2 -> f32x2, but f32x2 isn't really a valid type in WebAssembly (it is in CLIF).

I think we should use the .merge_lanes() approach here so that the types are f64x2 -> f32x4 and then in the instruction description we have to specify that the top two lanes are zeroed out. Or alternately, just explicitly put the types in.

Wasmtime GitHub notifications bot (Jun 03 2021 at 22:15):

abrown created PR review comment:

                With Fvdemote there is no constraint on having the result type have the same

Wasmtime GitHub notifications bot (Jun 04 2021 at 06:27):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (Jun 04 2021 at 19:56):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2021 at 19:56):

jlb6740 created PR review comment:

I've created new types with explicit lanes. Interesting enough after doing this I had to update the state push call in code_translator.rs. I also could have used split_lanes here though that would have been less constrained and semantically speaking just sounds vague on what that implies.

Wasmtime GitHub notifications bot (Jun 04 2021 at 20:28):

jlb6740 updated PR #2783 from implement_64_packed_promotelow_demote to main.

Wasmtime GitHub notifications bot (Jun 04 2021 at 22:50):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2021 at 22:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2021 at 22:59):

abrown merged PR #2783.

Last updated: Oct 23 2024 at 20:03 UTC