[llvm] r236171 - generalize binop reassociation; NFC
Sanjay Patel
spatel at rotateright.com
Thu Apr 30 08:07:10 PDT 2015
For the record, this patch was reverted along with r236031 by r236199.
We'll need to find a target-specific approach.
On Wed, Apr 29, 2015 at 7:00 PM, Jonathan Roelofs <jonathan at codesourcery.com
> wrote:
>
>
> On 4/29/15 5:52 PM, Sanjay Patel wrote:
>
>> 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
>>
>
> Commutative != Associative.
>
> It's worth duplicating and renaming that function, even if its contents
> end up being the same, so as to be precise in the 'literate programming'
> sense.
>
> An interesting, easy to understand counterexample would be a binop for
> average:
>
> avg(x,y)=(x+y)/2
>
> avg is commutative because:
>
> avg(x,y) = (x+y)/2 = (y+x)/2 = avg(y,x)
>
> but it is not associative because:
>
> avg(x, avg(y, z)) = (x + (y+z)/2)/2
> !=
> avg(avg(x, y), z) = ((x+y)/2 + z)/2
>
> (If such a thing existed as a target neutral opcode, which it does not.)
>
>
> Of the list in `isCommutativeBinop()`, the ones that are not obviously
> associative to me are:
>
> ADDC, ADDE, SADDO, UADDO
>
> I'm not sure what to do with the overflow/carry on those when
> re-associating, but I haven't spent much time thinking about it.
>
>
> Not on that list is FCOPYSIGN, which can be re-associated, but is not
> commutative:
>
> (FCOPYSIGN x (FCOPYSIGN y z)) ->
> (FCOPYSIGN (FCOPYSIGN x y) z)
>
> [definitely double check me on that one]
>
> I don't see any other target neutral ones that are like that, but I might
> be wrong.
>
> 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?
>>
>
> Seems reasonable to me... especially because it'll have to pull in the
> Options object to decide whether, for example, FADD is associative.
>
>
>> > 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. :)
>>
>
> I'd say, start with a small list of the ones you know for certain are
> associative, and grow it incrementally from there.
>
>
> Sidenote: if you make that change, then I think it subsumes this transform:
>
> // fold (fadd (fadd x, c1), c2) -> (fadd x, (fadd c1, c2))
>
> because it isn't doing any constant folding, and it's the same transform.
>
>
> Cheers,
>
> Jon
>
>
>>
>>
>> On Wed, Apr 29, 2015 at 5:16 PM, Jonathan Roelofs
>> <jonathan at codesourcery.com <mailto: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 <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>> --
>> Jon Roelofs
>> jonathan at codesourcery.com <mailto:jonathan at codesourcery.com>
>> CodeSourcery / Mentor Embedded
>>
>>
>>
> --
> 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/20150430/74d39a64/attachment.html>
More information about the llvm-commits
mailing list