Stream: git-wasmtime

Topic: wasmtime / PR #3188 Implement `Extractlane`, `UaddSat`, a...

Wasmtime GitHub notifications bot (Aug 16 2021 at 15:23):

dheaton-arm opened PR #3188 from implement-satarith to main:

Implemented the Extractlane, UaddSat, and UsubSat opcodes for the interpreter,
and added helper functions for working with SIMD vectors (extractlanes, vectorizelanes,
and binary_arith).

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 (Aug 16 2021 at 17:05):

afonso360 submitted PR review.

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

afonso360 created PR review comment:

Would it be possible to add a test run with the x86_64 and aarch64 backend to these tests as well? That way we get some more SIMD coverage on those targets. The s390x doesn't support SIMD yet, so no point in adding it.

Also could we move the test files to the runtests dir since we are testing run targets and the interpreter?

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

afonso360 submitted PR review.

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

afonso360 created PR review comment:

I could be wrong, but I think this annotation is only needed on x86_64 targets.

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

afonso360 edited PR review comment.

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

abrown submitted PR review.

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

abrown created PR review comment:

This might need to be more strict:

        assert!(ty.is_vector() && elem_ty.bytes() == 16);

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

abrown created PR review comment:

What do you think about implementing these instructions as: convert to larger type, use already existing add/sub, convert back to the right type with saturation (maybe with a new saturating ValueConversionKind). I can't remember how many instructions end up being saturating ones but if there are more it could reduce the size of DataValue.

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

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 08:39):

dheaton-arm submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 08:39):

dheaton-arm created PR review comment:

Looking at the list of opcodes, there's a few but given that a lot of them are signed/unsigned variants of each other (which I don't think will need re-implementing each time, as in this case for example), I think there'd be ~5 saturating ops in DataValue. We could do that suggestion though if preferred, which might make implementing opcodes like the narrow set easier later (the only question is if [I|U]128 is supposed to be supported; I see a few of the other functions in DataValue seem to ignore it and being the typically largest integer type would require different implementation).

Wasmtime GitHub notifications bot (Aug 17 2021 at 09:03):

dheaton-arm submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 09:03):

dheaton-arm created PR review comment:

Trying this I'm getting errors that i32x4 is an unsupported type for uadd_sat and usub_sat on the test run side -- should that be the case?

Wasmtime GitHub notifications bot (Aug 17 2021 at 10:47):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 10:47):

afonso360 created PR review comment:

Would it be easier to do a blanket impl of the Value trait like: impl<T: Value, const N: usize> Value for [T; N]? And have a function that would convert the SIMD values into/from the array?

I don't know how many of these functions we are going to need, but it might be worth considering in the future.

Although I think this would require rust 1.51 with min_const_generics. Which might be higher than our minimum requirements.

Wasmtime GitHub notifications bot (Aug 17 2021 at 10:49):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 10:49):

afonso360 created PR review comment:

    for (lhs, rhs) in arg0.iter().zip(arg1.iter()) {
        // The initial Value::int needs to be on a separate line so the
        // compiler can determine concrete types.
        let mut lhs: V = Value::int(*lhs, vector_type.lane_type())?;
        let mut rhs: V = Value::int(*rhs, vector_type.lane_type())?;

Wasmtime GitHub notifications bot (Aug 17 2021 at 10:52):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 17 2021 at 10:53):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 17 2021 at 10:55):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 17 2021 at 10:58):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 17 2021 at 19:22):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 19:22):

afonso360 created PR review comment:

Hmm, according to the docs, it should work.

However, the reason that it probably doesn't work is that its not included in the WASM SIMD Spec.

What we usually do in these cases is separate the testcases in two files. 8x16 / 16x8 in one file with the run targets, and i32x4 / i64x2 (could you add testcases for i64x2 as well?) into another with just the interpreter.

I also usually file an issue stating that these are not implemented in the x86_64 and aarch64 backends. We've been finding a lot of code that should compile but doesn't since we started adding these tests instead of relying on the wasm test suite.

Wasmtime GitHub notifications bot (Aug 17 2021 at 19:29):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 17 2021 at 19:30):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 18 2021 at 10:00):

dheaton-arm updated PR #3188 from implement-satarith to main.

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

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2021 at 13:33):

dheaton-arm updated PR #3188 from implement-satarith to main.

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

dheaton-arm requested abrown for a review on PR #3188.

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

cfallin submitted PR review.

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

cfallin created PR review comment:

I think it might be slightly preferable here to use SmallVec, perhaps SmallVec<[i128; 1]> or similar (and typedef it so we can easily change it) -- while we don't expect the interpreter to be fast, it's still good to avoid an unnecessary heap allocation per execution of an instruction.

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

abrown submitted PR review.

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

abrown created PR review comment:

Yeah, it sounds like the current approach will be fine.

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

abrown submitted PR review.

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

abrown created PR review comment:

Thanks for adding these instructions!

Wasmtime GitHub notifications bot (Aug 19 2021 at 10:24):

dheaton-arm submitted PR review.

Wasmtime GitHub notifications bot (Aug 19 2021 at 10:24):

dheaton-arm created PR review comment:

Is this specifically for extractlanes or would this also apply to the input to vectorizelanes? Just want to check in case you wanted basically all the Vecs in this section of code changing, whilst I'm thinking that parameters aren't an issue (which I may be wrong about) and Vec parameters seem more general / flexible (accepting both SmallVec and Vec).

Wasmtime GitHub notifications bot (Aug 19 2021 at 15:10):

dheaton-arm updated PR #3188 from implement-satarith to main.

Wasmtime GitHub notifications bot (Aug 19 2021 at 19:25):

cfallin created PR review comment:

I guess the advice applies generally. Specifically, this:

Vec parameters seem more general / flexible (accepting both SmallVec and Vec)

is not quite right; Vec is a specific type that requires its contents to be heap-allocated. The more idiomatic Rust type for "any array of values in memory" would be a slice, i.e. &[128], which one can get with &vec[..] or &smallvec[..]; but that only works for inbound args because it has no ownership. Returning a SmallVec is a technique we use elsewhere pretty successfully so I think it's' reasonable here too :-)

Wasmtime GitHub notifications bot (Aug 19 2021 at 19:25):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 19 2021 at 19:25):

cfallin edited PR review comment.

Wasmtime GitHub notifications bot (Aug 23 2021 at 08:15):

dheaton-arm submitted PR review.

Wasmtime GitHub notifications bot (Aug 23 2021 at 08:15):

dheaton-arm created PR review comment:

Ah, I was basing that statement off testing where using a Vec parameter but passing a SmallVec was accepted by the compiler and built successfully, but the inverse (using a SmallVec parameter but passing a Vec) was blocked by the compiler. I wasn't sure about if that made the SmallVec behave like a Vec with regards to heap allocations, however, hence my question.

Returning a SmallVec is a technique we use elsewhere pretty successfully

I understood that, and changed it to behave like that in 0089e15; I was more wondering about the input parameters to vectorizelanes. I'm gathering from your response though that the preference is for those to be a slice?

Wasmtime GitHub notifications bot (Aug 23 2021 at 12:01):

dheaton-arm updated PR #3188 from implement-satarith to main.

Wasmtime GitHub notifications bot (Aug 23 2021 at 12:17):

dheaton-arm edited PR review comment.

Wasmtime GitHub notifications bot (Aug 23 2021 at 16:28):

dheaton-arm requested cfallin for a review on PR #3188.

Wasmtime GitHub notifications bot (Aug 24 2021 at 04:09):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 24 2021 at 08:47):

dheaton-arm updated PR #3188 from implement-satarith to main.

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:03):

abrown merged PR #3188.

Last updated: Dec 23 2024 at 12:05 UTC