[llvm] r236171 - generalize binop reassociation; NFC
Jonathan Roelofs
jonathan at codesourcery.com
Wed Apr 29 18:00:34 PDT 2015
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
More information about the llvm-commits
mailing list