[PATCH][X86] Teach the backend how to simplify/canonicalize dag nodes introduced during type legalization.

[Andrea Di Biagio]

Hi Nadav,
thanks for the useful feedback.

On Thu, May 29, 2014 at 8:55 PM, Nadav Rotem <nrotem at apple.com> wrote:
> Hi Andrea,
>
> Thanks for working on this.
>
> +  // Type legalization might introduce new shuffles in the DAG.
> +  // Fold (VBinOp (shuffle (A, Undef, Mask)), (shuffle (B, Undef, Mask)))
> +  //   -> (shuffle (VBinOp (A, B)), Undef, Mask).
>
> There are other places in DAGCombine where we sink or hoist shuffles across binary operations. I think that SimplifyBinOpWithSameOpcodeHands does something similar.
>

True, we do a similar transformation for AND/OR/XOR dag nodes.
However, it seems to me that method '
SimplifyBinOpWithSameOpcodeHands' is only safe to be called on
AND/OR/XOR dag nodes. For example, some of the rules work under the
assumption that AND/OR/XOR dag nodes are indifferent to swizzle
operation.
The rule I tried to add with this patch would be more generic (for
example, it can be used to simplify an ADD or a SUB). That's why I
thought it was reasonable to have it in 'SimplifyVBinOp'.

> +  // During Type Legalization, when promoting illegal vector types,
> +  // the backend might introduce new shuffle dag nodes and bitcasts.
> +  //
> +  // This code performs the following transformation:
> +  // fold: (shuffle (bitcast (BINOP A, B)), Undef, <Mask>) ->
> +  //       (shuffle (BINOP (bitcast A), (bitcast B)), Undef, <Mask>)
>
> I think that your approach is reasonable but there may be other solutions. Have you considered handling bit-casted binary operations before type legalization?
>
> Clang is bit-casting <2 x float> to a double to implement the calling convention. If you are working in a Jitted environment and you don’t care about calling external functions then you can disable the bitcasting at the clang level.

Right, if I disable the bitcasting we don't get any shuffles.
When adding two <2 x i32> values, both CopyFromReg and CopyToReg use
v2i64 register types.
basically, rather than producing the sequence
  (v2i32: bitcast (f64,ch: CopyFromReg (Chain), vreg))
I get
  (v2i32: truncate (v2i64,ch: CopyFromReg (Chain), vreg)).

I can try to see if it makes sense to have a target specific combine
that attempts to convert the sequence bitcast+copyfromreg to a
truncate+copyfromreg. I am not sure if that would be safe to do in
general; (I think) it should be safe in the case of a bitcast from f64
-> v2i32; I am not sure it would be ok in other cases though. Is this
what you had in mind when you said that there might be other
solutions?

Cheers,
Andrea

>
> Thanks,
> Nadav
>
>
> On May 29, 2014, at 11:51 AM, Andrea Di Biagio <andrea.dibiagio at gmail.com> wrote:
>
>> Hi,
>>
>> This patch teaches the backend how to simplify/canonicalize dag node
>> sequences normally introduced by the backend when promoting dag nodes
>> with an illegal vector types.
>>
>> Example:
>>
>> define double @foo(double %A, double %B) {
>>  %1 = bitcast double %A to <2 x i32>
>>  %2 = bitcast double %B to <2 x i32>
>>  %add = add <2 x i32> %1, %2
>>  %3 = bitcast <2 x i32> %add to double
>>  ret double %3
>> }
>>
>> All the bitcasts in function @foo are promoted to type MVT::v2i64,
>> since type MVT::v2i32 is not a legal type. For the same reason, the
>> integer result of the vector add node also promoted.
>>
>> Type promotion might introduce new build_vector nodes (which are then
>> combined into shuffles) and bitcast operations. This is what happens
>> for example with function 'foo' that is compiled to the following
>> assembly sequence (using -mcpu=corei7):
>>  pmovzxdq  %xmm0, %xmm0  # promotion from <2 x i32> to <2 x i64>
>>  pmovzxdq  %xmm1, %xmm1  # promotion from <2 x i32> to <2 x i64>
>>  paddq %xmm0, %xmm1         # promoted to a legal add of type <2 x i64>
>>  pshufd $8, %xmm1, %xmm0  # xmm0 = xmm1[0,2,u,u]
>>
>> Ideally, the backend should be able to understand that the code
>> sequence above can be simplified into a single instruction:
>>  paddd %xmm0, %xmm1
>>
>> This patch adds two new combine rules:
>> 1)
>>  fold (shuffle (bitcast (BINOP A, B)), Undef, <Mask>) ->
>>       (shuffle (BINOP (bitcast A), (bitcast B)), Undef, <Mask>)
>>
>> 2)
>>  fold (BINOP (shuffle (A, Undef, <Mask>)), (shuffle (B, Undef, <Mask>))) ->
>>       (shuffle (BINOP A, B), Undef, <Mask>).
>>
>> The goal is to simplify the dag node sequence when dealing with 64-bit
>> vector types.
>>
>> Both rules are only triggered on the type-legalized DAG.
>> In particular, rule 1. is a target specific combine rule that attempts
>> to sink a bitconvert into the operands of a binary operation.
>> Rule 2. is a target independet rule that attempts to move a shuffle
>> immediately after a binary operation.
>>
>> With this patch, all the functions from test 'combine-64bit-vbinop.ll'
>> (a new test) are now strongly simplified.
>> In the case of function @foo (from the example), the backend now
>> correctly generates a single 'paddd' instruction.
>>
>> Please let me know if ok to submit.
>>
>> Thanks,
>> Andrea Di Biagio
>> <patch-dagcombine.diff>
>