Use getSplatValue.

Thanks. This LGTM

Thanks. LGTM if you fix the v2i32 pattern to be like the others.

Do And and Xor reductions work in the same way, with the same costs? Can we do those at the same time too?

For addp(addlp(x)) ->addv(x), addv genereates i32 output from v4i32 input and i16 output from v4i16 input. However, addp(addlp(x)) generates i64 output from v4i32 input and i32 output from v4i16 input. I think the outputs could not be same with big numbers input between addp(addlp(x)) and addv(x). If I missed something, please let me know.

Sorry about the very long delay. Other things came up, but I still think this is worthwhile. It feels a little odd to introduce a target node that isn't legal, but it helps in the lowering of truncates under MVE. It should allow us to, instead of expanding the trunc into a lot of lane extract/insert operations, lower them to two truncating stack stores and a reload (which I will do as a separate patch). We can then do the same for zext/sext, which should help improve the worst case lowering for a lot of zext/sext/trunc we currently see. And help with the lowering of nodes like VABD like we see here, and things like VMULL and certain vector reductions.

AArch64uaddv is a slightly overloaded term. It comes from a vecreduce and can be lowered to a number of things. I think for anything that is v2iX, it will produce an addp, not an addv. So we want to turn addp(addlp(x)) ->addv(x), but that is still the same pattern extended to v4i32->v2i64->i64 and v4i16->v2i32->i32 variants.

In D100464#2822499, @clin1 wrote:

spec2000/186 passes now -- thanks.

Thanks. I would expect 2 more types I think. v2i32 and v2i64? It may be better to create a new multiclass like SIMDAcrossLanesIntrinsic.

Reopening with adjusted code (and more test cases). The issues were related to the checks for all paths leading to the exit collectively postdominating the earlier dead store. But:

• A store that walked back to itself was ignored, effectively killing itself. This was this issue that came up from the reproducer. It is only true if the access is loop invariant, so it now uses an extra isGuaranteedLoopInvariant() check.
• Stores in blocks with later PO numbers were again effectively ignored. With loops they need to be handled, in this case by returning None.

It sounds like it needs something like SIMDAcrossLanesIntrinsic but for the combined uaddlp + addv patterns. That way it should be able to handle both the (AArch64uaddv(AArch64uaddlp(..)) form and the extract(insert((AArch64uaddv(AArch64uaddlp(..)))) form, hopefully.

This sounds OK to me, so long as the X86 numbers are not bonkers.

Can we add the other types too? It's good to add all the varieties if we can.

Thanks. LGTM

It is good point! I have tried below pattern following your suggestion. It seems to work. If you are ok, let me add below pattern in this patch.

let AddedComplexity = 10 in { 
def : Pat<(i32 (extractelt
                 (v4i32 (AArch64uaddv (v4i32 (AArch64uaddlp (v8i16 V128:$op))))),
                 (i64 0))),
          (UADDLVv8i16v V128:$op)>;
}

Do you mind doing this as a new patch? As it does feel logically separable. If we can test them, it would be good to add the various other sizes too. And, I'm not sure about this, but maybe it doesn't need to start from the extract, and can produce a INSERT_SUBREG like some of the other patterns do (like the ones from SIMDAcrossLanesIntrinsic). That might remove the need for the added complexity, and the INSERT_SUBREG / EXRACT_SUBREG should all get cleared up later in the pipeline.

Looks sensible to me, thanks

Nice patch. It's a big combine, but we certainly have bigger elsewhere. Sometimes it's possible to do things like this as smaller pieces, that add up to the same thing. Speaking of which, would it be possible to combine (in a separate patch, maybe as a tablegen pattern) addv (uaddlp x) -> uaddlv x ?

Thanks. LGTM

It seems like a simpler overall route to remove the assert from the vectorizer, and allow it to deal with invalid costs. But I am not blocking this patch.

Costs look good, as far as I can tell.

I got confused by the _fixed vs _register vs _UPD for a while, but I see that's pre-existing :)

In D103882#2805812, @sdesmalen wrote:

In D103882#2805707, @dmgreen wrote:

Why 9999? Why not an invalid cost? I thought that was what invalid was for, so we didn't need hacky random numbers like this.

We've chosen not to conflate legalization and cost-modelling, so that the cost-model must return a valid cost when the legalization phase says that a given VF is legal to use. The benefit of that is that it guards the requirement to have a complete (albeit not necessarily accurate) cost-model. For SVE a VF of vscale x 1 is legal in principle, meaning that we should be able to code-generate it. But because our code-generator is incomplete, we want to avoid using these VFs. I haven't added an interface to query the minimum legal VF, since I know we'll remove that interface at some point. The LV will expect all costs calculated to be Valid and will fail this assertion otherwise. So basically return <magic number> is just a temporary stop-gap to avoid failing that (and other) assertion failures.

Does that make more sense?

Oh so it does. Thanks for the report. I thought we had a test case for that...

Nice one. Sounds funky.

Why 9999? Why not an invalid cost? I thought that was what invalid was for, so we didn't need hacky random numbers like this.

In D103755#2800920, @RKSimon wrote:

Funnily enough I was wondering about this pattern the other day as a followup to D98778...

Should we always be folding unaryop(splat(x)) -> splat(unaryop(x)) if the unaryop is legal/custom on the scalar type? And then maybe extend that to binop(splat(x),splat(y)) -> splat(binop(x,y)) as well?

Rebase over D103756

Thanks Folks.

Added two new test cases, mov_int8_1234 that does like you said i8 <1,2,3,4,1,2,3,4,..> and mov_int32_16908546 which is 0x1020102 VDUP'd as a i16.

Update comment

In D103756#2800940, @RKSimon wrote:

Whilst I'm here also add a call to peekThroughBitcasts as bitcast Zero is still Zero, and removed a related TODO comment from isOneOrOneSplat as the bitcast of 1 isn't always still 1.

Is this necessary for any of the test changes? I just wonder whether we should add this separately.

Thanks. LGTM

UnJ isn't enabled by default upstream yet in the pass manager, but it should be OK to set the option in the target.

Sounds good. Now with a getWhileLoopStartTargetBB used in more places.

Thanks. LGTM.