[llvm] r236171 - generalize binop reassociation; NFC

[Sanjay Patel]

Hi Jon -

Thanks for the review!

> Perhaps add a "the opcode really is associative" assertion?

I just discovered SelectionDAG::isCommutativeBinOp(). Do you see any
problems using that as the assert check? If we can use this fold on all of
those ops, then I think we'd hoist it's use even higher so that it's
checked from some common function rather than each potential binop's
visitX() function?

> Is it sufficient for N00:k to be any binop? Can it be relaxed even
further?

Good point; I missed that generalization. I don't see a problem with this:
x * y + z + w -> (x * y) + (z + w)

I want to believe we can do it, but I'm not sure yet...particularly if
isCommutativeBinOp() is the full list of potential fold
candidates...because I have no knowledge of some of those opcodes yet. :)



On Wed, Apr 29, 2015 at 5:16 PM, Jonathan Roelofs <jonathan at codesourcery.com
> wrote:

>
>
> On 4/29/15 4:30 PM, Sanjay Patel wrote:
>
>> Author: spatel
>> Date: Wed Apr 29 17:30:02 2015
>> New Revision: 236171
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=236171&view=rev
>> Log:
>> generalize binop reassociation; NFC
>>
>> Move the fold introduced in r236031:
>> http://reviews.llvm.org/rL236031
>>
>> to its own helper function, so we can use it for other binops.
>>
>> This is a preliminary step before partially solving:
>> https://llvm.org/bugs/show_bug.cgi?id=21768
>> https://llvm.org/bugs/show_bug.cgi?id=23116
>>
>>
>> Modified:
>>      llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>
>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=236171&r1=236170&r2=236171&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Wed Apr 29
>> 17:30:02 2015
>> @@ -7647,6 +7647,33 @@ SDValue DAGCombiner::visitFSUBForFMAComb
>>     return SDValue();
>>   }
>>
>> +static SDValue ReassociateBinops(SDNode *N, SelectionDAG &DAG) {
>> +  assert(N->getNumOperands() == 2 && "Invalid node for binop
>> reassociation");
>> +  SDValue N0 = N->getOperand(0);
>> +  SDValue N1 = N->getOperand(1);
>> +  EVT VT = N->getValueType(0);
>> +  SDLoc DL(N);
>> +  unsigned Opcode = N->getOpcode();
>>
>
> Perhaps add a "the opcode really is associative" assertion?
>
>  +
>> +  // Canonicalize chains of this operation to LHS to allow the following
>> fold.
>> +  if (N0.getOpcode() != Opcode && N1.getOpcode() == Opcode)
>> +    return DAG.getNode(Opcode, DL, VT, N1, N0);
>> +
>> +  // Convert a chain of 3 dependent operations into 2 independent
>> operations
>> +  // and 1 dependent operation:
>> +  //  (op N0: (op N00: (op z, w), N01: y), N1: x) ->
>> +  //  (op N00: (op z, w), (op N1: x, N01: y))
>> +  if (N0.getOpcode() == Opcode && N0.hasOneUse() && N1.getOpcode() !=
>> Opcode) {
>> +    SDValue N00 = N0.getOperand(0);
>> +    if (N00.getOpcode() == Opcode) {
>>
>
> Realizing that your change is NFC...
>
> Why do we care that N00's opcode matches the other two? N00:k doesn't
> really participate in the re-association of N01:y and N1:x:
>
>   (op N0: (op N00: k, N01: y), N1: x) ->
>   (op N00: k, (op N1: x, N01: y))
>
> or in infix notation without the N's, that is:
>
>    (k +  y) + x ->
>     k + (y  + x)
>
> Is it sufficient for N00:k to be any binop? Can it be relaxed even further?
>
>
> Cheers,
>
> Jon
>
>
>  +      SDValue N01 = N0.getOperand(1);
>> +      SDValue NewOp = DAG.getNode(Opcode, DL, VT, N1, N01);
>> +      return DAG.getNode(Opcode, DL, VT, N00, NewOp);
>> +    }
>> +  }
>> +  return SDValue();
>> +}
>> +
>>   SDValue DAGCombiner::visitFADD(SDNode *N) {
>>     SDValue N0 = N->getOperand(0);
>>     SDValue N1 = N->getOperand(1);
>> @@ -7781,24 +7808,10 @@ SDValue DAGCombiner::visitFADD(SDNode *N
>>                              N0.getOperand(0), DAG.getConstantFP(4.0, DL,
>> VT));
>>         }
>>       }
>> -
>> -    // Canonicalize chains of adds to LHS to simplify the following
>> transform.
>> -    if (N0.getOpcode() != ISD::FADD && N1.getOpcode() == ISD::FADD)
>> -      return DAG.getNode(ISD::FADD, DL, VT, N1, N0);
>>
>> -    // Convert a chain of 3 dependent operations into 2 independent
>> operations
>> -    // and 1 dependent operation:
>> -    //  (fadd N0: (fadd N00: (fadd z, w), N01: y), N1: x) ->
>> -    //  (fadd N00: (fadd z, w), (fadd N1: x, N01: y))
>> -    if (N0.getOpcode() == ISD::FADD &&  N0.hasOneUse() &&
>> -        N1.getOpcode() != ISD::FADD) {
>> -      SDValue N00 = N0.getOperand(0);
>> -      if (N00.getOpcode() == ISD::FADD) {
>> -        SDValue N01 = N0.getOperand(1);
>> -        SDValue NewAdd = DAG.getNode(ISD::FADD, DL, VT, N1, N01);
>> -        return DAG.getNode(ISD::FADD, DL, VT, N00, NewAdd);
>> -      }
>> -    }
>> +    if (SDValue Reassociated = ReassociateBinops(N, DAG))
>> +      return Reassociated;
>> +
>>     } // enable-unsafe-fp-math
>>
>>     // FADD -> FMA combines:
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
> --
> Jon Roelofs
> jonathan at codesourcery.com
> CodeSourcery / Mentor Embedded
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150429/b31f30d2/attachment.html>