[PATCH] [AArch64] Fix a pattern match failure caused by creating improper CONCAT_VECTOR.

[Tim Northover]

Hi Kevin,

>> Isn't the key type here VT.getVectorElementType()? I'm not sure
>> I see the logic of caring about any AssertSext/AssertZext input:
>> we're just going to be discarding the bits anyway in your example.
>>
>> In the reverse case (say extracting from v4i16 and inserting into
>> v4i32), as far as I can see EXTRACT_VECTOR_ELT will basically
>> be doing an anyext, so we don't have to care there either.
>>
>> Or do you have examples where this kind of check is needed?
>
> If input cannot pass element type check, we can simply get function
> return and the incorrect concat_vector would not be generated.

Fair enough, *something* clearly has to be done; I'm not disputing that.

> But
> from the result, I saw lots of MOV and INS instructions are generated
> for this build_vector. To get better result, I added additional codes here
> specically for AssertSext /AssertZext, wishing to generate single UZIP1
> instead. For AssertSext /AssertZext, only low bits of each element are
> valid, so for a v2i32 AssertSext node asserting holds i16, it can bitcast
> to v4i16, which lane 0, 2 are matching prior lanes and lane 1, 3 are
> undefined.

I think that works regardless of whether the AssertSext is present or
not. All the AssertZext and AssertSext nodes tell us is that the lanes
we *don't* care about are going to be either 0 or -1 (and that's
assuming they match up with the vector we're building, they're even
less useful otherwise).

>> Instead of mangling the actual BUILD_VECTOR node, perhaps we should
>> create a similar OffsetMultipliers (say) variable and just record what we've
>> done for later in the function.
>
> You mean rename Pow to OffsetMultipliers? I will do that in next version.

I mean something a bit bigger: completely remove the
"DAG.getNode(ISD::BUILD_VECTOR, ...)" code and instead save the
information needed to extract the correct lanes later, in an "int
OffsetMultipliers[2] = { 1, 1 };" variable.

Purely for discussion purposes, I've attached a quick hatchet-job I've
done on the patch along the lines I'm suggesting (clearly unpolished).
It gets the test you wrote correct, without so many special cases. I
assume you had something similar before you settled on using
AssertZext and AssertSext, do you have an example of why it's not the
right solution?

Cheers.

Tim.

- {F73262, layout=link}

http://reviews.llvm.org/D4080