Stream: git-wasmtime

Topic: wasmtime / PR #3031 Add extend-add-pairwise instructions x64

Wasmtime GitHub notifications bot (Jun 24 2021 at 23:11):

jlb6740 opened PR #3031 from extend-add-pairwise-x64 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 (Jun 24 2021 at 23:17):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 25 2021 at 03:27):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 29 2021 at 06:21):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 30 2021 at 04:38):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 30 2021 at 04:49):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 30 2021 at 04:54):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 30 2021 at 05:26):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 30 2021 at 05:32):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 30 2021 at 05:44):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 30 2021 at 05:48):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 30 2021 at 06:16):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jun 30 2021 at 16:25):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2021 at 16:25):

akirilov-arm created PR review comment:

An alternative approach is to introduce a binary pairwise addition IR operation, e.g. iadd_pairwise, which sounds like a more useful primitive operation - it could be used both by other existing Wasm instructions like i32x4.dot_i16x8_s and possible future additions (some of which have already been discussed - see here for an example); in that case extended_pairwise_add_signed would become iadd_pairwise(swiden_low, swiden_high). We need to do some pattern-matching for optimal AArch64 code generation (which is a single instruction), but I am not sure about other architectures - any thoughts from the people working on the backends?

cc @abrown @cfallin @uweigand

Wasmtime GitHub notifications bot (Jun 30 2021 at 16:36):

akirilov-arm edited PR review comment.

Wasmtime GitHub notifications bot (Jun 30 2021 at 17:15):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2021 at 17:15):

abrown created PR review comment:

A general comment: in the past we were implicitly following the "make a new CLIF instruction for each Wasm SIMD instruction" paradigm so a lot of existing CLIF instructions could be split up in the same way you describe above. I think this is a thing that should be done (it just seems to make sense to simplify CLIF even at the risk of a few extra cycles during lowering) but I don't feel very strongly about it so I would be fine with either:

• we do the pattern matching now AND we start a review of the existing SIMD instructions in a separate issue to clean up CLIF
• we just start the review of existing SIMD instructions now and eventually remove the unnecessary CLIF instructions and switch to more pattern matching; this would mean following the "make a new CLIF instruction for each Wasm SIMD instruction" for the last few outstanding instructions

In summary, I don't mind if we do iadd_pairwise now or later, but I do think we need to commit to doing the same to the rest of SIMD CLIF.

Wasmtime GitHub notifications bot (Jun 30 2021 at 17:39):

akirilov-arm created PR review comment:

I forgot to link to the discussion in #2982 for context.

@abrown While I don't have any strong feelings either, in #3044 I have already started with the first of your suggested approaches. In fact, the only Wasm SIMD operations that are not already covered by merged code or pull requests are the family of i*.extmul_*_i* operations, for which the specification text itself provides a simple way to express them in terms of other basic operations.

I agree that opening an issue for CLIF clean-up is definitely one of the steps forward, no matter what we decide to do.

Wasmtime GitHub notifications bot (Jun 30 2021 at 17:39):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2021 at 18:13):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2021 at 18:13):

abrown created PR review comment:

Opened https://github.com/bytecodealliance/wasmtime/issues/3045 for this.

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

cfallin submitted PR review.

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

cfallin created PR review comment:

Just commented over in #3045 with thoughts on the general question; for this specific case I think it does seem reasonable to split into the pairwise add with extends on the inputs. But let's see what we come to over there!

Wasmtime GitHub notifications bot (Jul 01 2021 at 06:26):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 07 2021 at 06:34):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 08 2021 at 20:21):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 08 2021 at 20:24):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 09 2021 at 02:42):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 09 2021 at 03:07):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jul 09 2021 at 03:07):

jlb6740 created PR review comment:

@akirilov-arm @cfallin Hi .. so the suggestion here is to remove the new IR instructions extended_pairwise_add_signed and extended_pairwise_add_unsigned and instead create one called iadd_pairwise. When lowering .. we would keep the lowering code that is there now, but pattern match for the instruction and type during lowering in order to get the input, or is the suggestion to scrape that lowering code too, actually lower swiden_low and swiden_high where we then are using the output of those particular instructions to be input to iadd_pairwise which needs to then be a new implementation from what is there now, essentially creating a new patch? I look at the change made in #2982 and it really is just avoiding creating another IR instruction while maybe adding a little logic of doing the matching, but keeps the original sequence. Here I am not sure if the suggestion is to go even further?

Wasmtime GitHub notifications bot (Jul 09 2021 at 03:10):

jlb6740 edited PR review comment.

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

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 10 2021 at 01:13):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 13 2021 at 16:11):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 13 2021 at 16:11):

akirilov-arm created PR review comment:

IMHO the handling should be equivalent to the one for the fcvt_from_uint operation in #2982 - that is, there should be a code path that handles iadd_pairwise irrespective of its inputs, to serve as a fallback. Then right before that path there should be a check if the inputs match the pattern; if they do, execution should proceed through your current lowering code for extended_pairwise_add_*.

BTW @sparker-arm already has the common iadd_pairwise implementation and the AArch64-specific bits, but he is waiting for this pull request to get merged, so that you do not step on each other's toes. In the meantime, he may be able to offer some advice.

Wasmtime GitHub notifications bot (Jul 13 2021 at 16:42):

sparker-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 13 2021 at 16:42):

sparker-arm created PR review comment:

Yeah, I don't mind posting this up if people are interested.

Wasmtime GitHub notifications bot (Jul 14 2021 at 02:51):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2021 at 02:51):

jlb6740 created PR review comment:

Hi @spaker-arm .. can you post here?@akirilov-arm I still am not sure I understand what changes are in mind. Currently in code_translators there is this new instruction extended_pairwise_add_signed called here:

    Operator::I16x8ExtAddPairwiseI8x16S => {
        let a = pop1_with_bitcast(state, I8X16, builder);
        state.push1(builder.ins().extended_pairwise_add_signed(a))
    }

and new instruction extended_pairwise_add_unsigned called here:

    Operator::I16x8ExtAddPairwiseI8x16U => {
        let a = pop1_with_bitcast(state, I8X16, builder);
        state.push1(builder.ins().extended_pairwise_add_unsigned(a))
    }

You are saying to instead just call one instruction called iadd_pairwise and then lower from there? I think this is what you are saying and I'll go ahead and repush this.

Wasmtime GitHub notifications bot (Jul 14 2021 at 02:51):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 14 2021 at 02:52):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 14 2021 at 02:52):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 14 2021 at 02:55):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 14 2021 at 02:55):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 14 2021 at 02:56):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 14 2021 at 08:29):

sparker-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 14 2021 at 08:29):

sparker-arm created PR review comment:

Sure, I'll post the whole thing up later today. But just to point out that we'll know the signedness because of the [u|s]widen inputs, for example:

        Operator::I16x8ExtAddPairwiseI8x16S => {
            let a = pop1_with_bitcast(state, I8X16, builder);
            let widen_low = builder.ins().swiden_low(a);
            let widen_high = builder.ins().swiden_high(a);
            state.push1(builder.ins().iadd_pairwise(widen_low, widen_high));
        }

This creates a bit more work in the AArch64 backend, but it does also mean that we can fallback to an ADDP when we don't successfully match the extending inputs. As a side queston, unless I'm mistaken wasm doesn't have a horizontal add - does anyone know why?

Wasmtime GitHub notifications bot (Jul 29 2021 at 18:59):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 30 2021 at 00:45):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 30 2021 at 04:07):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 30 2021 at 04:09):

jlb6740 requested akirilov-arm for a review on PR #3031.

Wasmtime GitHub notifications bot (Jul 30 2021 at 04:09):

jlb6740 requested cfallin for a review on PR #3031.

Wasmtime GitHub notifications bot (Jul 30 2021 at 08:13):

sparker-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 08:13):

sparker-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 08:13):

sparker-arm created PR review comment:

I think it would be good to have a clearer definition of the semantics, this currently doesn't tell me which elements make a pair.

Wasmtime GitHub notifications bot (Jul 30 2021 at 08:13):

sparker-arm created PR review comment:

Sorry, I don't know anything about the ISA... but since you're using 'src' here, shouldn't you be checking that the input to the swiden_low and swiden_high is the same?

Wasmtime GitHub notifications bot (Jul 30 2021 at 16:33):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 16:33):

cfallin created PR review comment:

These nested conditionals could be written together in one level, I think; this would also avoid the subtle asymmetry (possibly leading to a bug with later additions) that the else if attaches only to the outer if. Something like:

if let (Some(swiden_low), Some(swiden_high)) = (matches_input(...), matches_input(...)) {

Wasmtime GitHub notifications bot (Jul 30 2021 at 16:33):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 16:33):

cfallin created PR review comment:

+1, this needs to be part of the condition above (where we check input opcodes) such that we fall back to generic codegen otherwise, as it's perfectly legal to pairwise-add extends of two different values.

Wasmtime GitHub notifications bot (Jul 30 2021 at 16:33):

cfallin created PR review comment:

At this point we can now remove x64_should_panic entirely, since it always returns false, right?

Wasmtime GitHub notifications bot (Jul 30 2021 at 16:33):

cfallin created PR review comment:

Are these still the old opcodes?

Wasmtime GitHub notifications bot (Jul 30 2021 at 16:33):

cfallin created PR review comment:

I think we need an else branch here to handle a pairwise add without the extends, no? Or at least an unimplemented!() if we don't support that opcode by itself.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:00):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:00):

jlb6740 created PR review comment:

Yes .. I was not sure if we wanted to leave it in for future instructions .. relaxed SIMD maybe? But at the very least I was going to remove the match here. I can just remove the function and find the call and remove that to.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:10):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:10):

jlb6740 created PR review comment:

@sparker-arm @cfallin .. This is exactly what I was thinking about. This is the tension between treating a clif as a 1:1 wasm instruction mapping versus the decomposition approach. I didn't add the check because as noted, it is perfect legal for swiden_low and swiden_high to have to not be the same per the definition of iadd_pairwise. Here, we are not lowering ext_addpairwise which would require the condition that the input be the same ... we are lowering iadd_pairwise. I was concerned that some exercise or fuzz testing or something lowering just this cliff instruction could be invalidated incorrectly. I can add the check as I want this to merge, but I do think we need to think about or even audit the consistency of similar checks and whether they map to the clif instruction that we are actually lowering or the wasm instruction that we are intending to lower.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:12):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:12):

jlb6740 created PR review comment:

Glad you caught this. Yes, was going to do one more once through but may have missed this. Surprised it doesn't warn?

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:15):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:27):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:27):

cfallin created PR review comment:

Here, we are not lowering ext_addpairwise which would require the condition that the input be the same ... we are lowering iadd_pairwise

Well, that's true in the outer scope, but the instructions that this case is emitting are specifically for the same-input case, no?

IMHO it's fine for now to fall back to unimplemented!() if we know for sure that we won't generate an iadd_pairwise with other (arbitrary) inputs, but at some point we should fill in that case too, for completeness.

should audit ...

I agree! I have lots of thoughts on verification that this text box is too small to contain :-) IMHO this is the next big effort after getting current projects (including some sort of isel DSL) landed.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:28):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:28):

cfallin created PR review comment:

Ah, yeah, I think it's fine to remove the whole helper and put it back later if we need it.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:29):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 17:29):

cfallin created PR review comment:

Me too; @abrown I guess we're not building the CLIF interpreter by default in any CI target?

Wasmtime GitHub notifications bot (Jul 30 2021 at 20:36):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 30 2021 at 20:36):

abrown created PR review comment:

I thought we were: there are test interpret filetest that should use it and @afonso360 has a fuzz target that should use it.

Wasmtime GitHub notifications bot (Jul 30 2021 at 20:36):

abrown edited PR review comment.

Wasmtime GitHub notifications bot (Jul 31 2021 at 00:21):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 31 2021 at 00:40):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 31 2021 at 00:45):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 31 2021 at 01:07):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 31 2021 at 01:32):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 31 2021 at 01:39):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 31 2021 at 01:40):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 31 2021 at 02:17):

jlb6740 requested cfallin for a review on PR #3031.

Wasmtime GitHub notifications bot (Jul 31 2021 at 02:41):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 31 2021 at 02:41):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 31 2021 at 02:41):

cfallin created PR review comment:

I don't think this is testing what we want to test -- this is just checking that we grabbed input 0 on each of the swiden ops (which is always true by construction, since we built swiden_input just above).

What we really want is to put_input_in_reg(ctx, swiden_input[1]), and then assert that we have the same register, I think.

Also, if the inputs do not match, we should fail more gracefully than with an assert, I think; asserts imply a compiler bug, whereas the different-inputs case is really just unimplemented behavior, right? So perhaps we could do (after getting src0 and src1 regs) if src0 != src1 { unimplemented!("iadd_pairwise not implemented for general case with different inputs"); } or something like that?

Wasmtime GitHub notifications bot (Jul 31 2021 at 02:41):

cfallin created PR review comment:

s/adjcent/adjacent/

and I think s/lines/lanes/

Wasmtime GitHub notifications bot (Jul 31 2021 at 18:37):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 31 2021 at 18:38):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jul 31 2021 at 18:38):

jlb6740 created PR review comment:

Sorry .. Yes. When I first did this I was not sure if that's what we wanted here and forgot to highlight that concern. Good catch. Also updated for the unimplemented vs assert comment.

Wasmtime GitHub notifications bot (Jul 31 2021 at 18:39):

jlb6740 updated PR #3031 from extend-add-pairwise-x64 to main.

Wasmtime GitHub notifications bot (Jul 31 2021 at 19:12):

jlb6740 requested cfallin for a review on PR #3031.

Wasmtime GitHub notifications bot (Jul 31 2021 at 19:12):

jlb6740 requested abrown for a review on PR #3031.

Wasmtime GitHub notifications bot (Aug 01 2021 at 04:14):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2021 at 04:14):

cfallin merged PR #3031.

Wasmtime GitHub notifications bot (Aug 01 2021 at 04:15):

cfallin submitted PR review.

Last updated: Oct 23 2024 at 20:03 UTC