Stream: git-wasmtime

Topic: wasmtime / PR #3070 Enable simd_extmul_* for AArch64

Wasmtime GitHub notifications bot (Jul 08 2021 at 15:42):

sparker-arm opened PR #3070 from simd-extmul-aarch64 to main:

Lower simd_extmul_[low/high][signed/unsigned] to [s|u]widen inputs to
an imul node.

This also reorganises the aarch64 'long mul' operations and their assembly.

Copyright (c) 2021, Arm Limited.

<!--

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 (Jul 09 2021 at 09:28):

sparker-arm updated PR #3070 from simd-extmul-aarch64 to main.

Wasmtime GitHub notifications bot (Jul 14 2021 at 17:44):

akirilov-arm requested cfallin for a review on PR #3070.

Wasmtime GitHub notifications bot (Jul 14 2021 at 20:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2021 at 20:48):

alexcrichton created PR review comment:

It seems like this and Umlal8 aren't actually generated in any lowerings (unless I'm missing something), but do you want to leave these in for completeness with possible future lowerings?

Wasmtime GitHub notifications bot (Jul 14 2021 at 20:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2021 at 20:48):

alexcrichton created PR review comment:

One possible way to structure this is to perhaps check for is_vector and i64x2 early on at the top of this block with early-returns if they match, and if that fails all the remaining cases share the logic of

                    let rn = put_input_in_reg(ctx, inputs[0], NarrowValueMode::None);
                    let rm = put_input_in_reg(ctx, inputs[1], NarrowValueMode::None);
                    let rd = get_output_reg(ctx, outputs[0]).only_reg().unwrap();

as a prefix.

Just a thought though, happy to defer to you who work on this much more than I!

Wasmtime GitHub notifications bot (Jul 15 2021 at 07:21):

sparker-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 15 2021 at 07:21):

sparker-arm created PR review comment:

Yes, exactly - thought it would be weird to just port the 32-bit version.

Wasmtime GitHub notifications bot (Jul 15 2021 at 07:37):

sparker-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 15 2021 at 07:37):

sparker-arm created PR review comment:

So even though I don't like the duplication, or how much conditional stuff is going on here, we would only be able to factor out one collection of these calls as the I128 case uses, ever so slightly, different calls to handle multiple registers. So I think I prefer the clarity.

Wasmtime GitHub notifications bot (Jul 26 2021 at 21:25):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 26 2021 at 21:25):

cfallin created PR review comment:

Can we add a doc-comment here describing the return tuple? E.g. it's not clear to me reading at this point what the bool denotes, until I notice the low/high pattern below in the implementation.

Wasmtime GitHub notifications bot (Jul 26 2021 at 21:25):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 12:18):

sparker-arm updated PR #3070 from simd-extmul-aarch64 to main.

Wasmtime GitHub notifications bot (Jul 28 2021 at 18:10):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 18:10):

cfallin merged PR #3070.

Last updated: Nov 22 2024 at 17:03 UTC