Stream: git-wasmtime

Topic: wasmtime / PR #3384 Implement `bmask` in AArch64

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 13:55):

afonso360 opened PR #3384 from aarch64-bmask to main:

Hey, this PR Implements bmask for all types in the AArch64 backend, as well as fixes for ireduce, breduce and bextend for some types.

We have to make some changes in the trampoline to make bool args with size larger than 1 work.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 15:47):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 15:47):

akirilov-arm created PR review comment:

I am afraid that this is not true for vector types; e.g. consider an operation from B8X8 to B16X8.

Unless both the input and the output have the same lane sizes (the number of lanes is always the same, as specified by the operation descriptions), you will have to write much more complicated logic, and will probably be unable to merge the scalar and vector code paths, as you do now.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 15:47):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 16:04):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 16:04):

afonso360 created PR review comment:

Right, I didn't really prepare this for those types (non 128 bit vectors). I haven't really seen them used anywhere so far.

Would it be acceptable to assert on 128bit vectors for now?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 17:32):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 17:32):

akirilov-arm created PR review comment:

No, just keep the block (lines 1864 - 1869) that you have removed. Of course, you will need to change the condition - I suppose to from_ty.is_vector() && (from_bits != 128 || from_bits != to_bits).

IMHO assertions should be used for verifying invariants guaranteed by the IR descriptions, e.g. checking that the number of input lanes is equal to the number of output lanes. That is the approach I have used in my recent changes.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 17:40):

akirilov-arm edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 17:43):

akirilov-arm edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 23 2021 at 17:43):

akirilov-arm edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 17:31):

afonso360 updated PR #3384 from aarch64-bmask to main.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:36):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:36):

akirilov-arm created PR review comment:

Given that to_bits > from_bits, we know that from_bits <= 64, so there is no need to calculate a minimum here.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:36):

akirilov-arm created PR review comment:

We could also be lowering Ireduce - see below.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:36):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:36):

akirilov-arm created PR review comment:

Remove this assertion - Breduce from B128 to B128 and Ireduce from I128 to I128 are also valid operations.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:36):

akirilov-arm created PR review comment:

I feel like this should be calculated in parallel with the bottom half, at the expense of a bit of code duplication, especially in the case of from_bits == to_bits.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:49):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:49):

akirilov-arm created PR review comment:

Even after your latest changes, this comment should be changed to Smaller scalar integers/booleans are....

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:52):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:52):

akirilov-arm created PR review comment:

Now that I think about it, Ireduce from I128 to I128 needs completely different treatment - the upper destination register is not a copy of the lower source register.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 24 2021 at 21:57):

akirilov-arm edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 15:50):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 15:50):

afonso360 created PR review comment:

The docs state that ireduce requires that the output type is smaller than the input type, so at most we only ever have a i64 output (if we get a i128 input). I've checked and the verifier enforces this. breduce has the same language in the docs.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 17:04):

akirilov-arm created PR review comment:

Well, then there is a contradiction because the documentation right now states:

The result type must have the same number of vector lanes as the input, and each lane must not have more bits that the input lanes.

In other words, there are two options - either the same number of bits per lane, or fewer. If indeed the verifier enforces the second option, then do you mind making the description less ambiguous? Same for Breduce.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 17:04):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 26 2022 at 16:03):

afonso360 closed without merge PR #3384.

Last updated: Nov 22 2024 at 16:03 UTC