<div dir="ltr">For the record, this patch was reverted along with r236031 by r236199. <br><br>We'll need to find a target-specific approach.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 29, 2015 at 7:00 PM, Jonathan Roelofs <span dir="ltr"><<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
On 4/29/15 5:52 PM, Sanjay Patel wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Jon -<br>
<br>
Thanks for the review!<br>
</blockquote>
<br></span>
:)<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Perhaps add a "the opcode really is associative" assertion?<br>
<br>
I just discovered SelectionDAG::isCommutativeBinOp(). Do you see any<br>
problems using that as the assert check? If we can use this fold on all<br>
</blockquote>
<br></span>
Commutative != Associative.<br>
<br>
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.<br>
<br>
An interesting, easy to understand counterexample would be a binop for average:<br>
<br>
avg(x,y)=(x+y)/2<br>
<br>
avg is commutative because:<br>
<br>
avg(x,y) = (x+y)/2 = (y+x)/2 = avg(y,x)<br>
<br>
but it is not associative because:<br>
<br>
avg(x, avg(y, z)) = (x + (y+z)/2)/2<br>
!=<br>
avg(avg(x, y), z) = ((x+y)/2 + z)/2<br>
<br>
(If such a thing existed as a target neutral opcode, which it does not.)<br>
<br>
<br>
Of the list in `isCommutativeBinop()`, the ones that are not obviously associative to me are:<br>
<br>
ADDC, ADDE, SADDO, UADDO<br>
<br>
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.<br>
<br>
<br>
Not on that list is FCOPYSIGN, which can be re-associated, but is not commutative:<br>
<br>
(FCOPYSIGN x (FCOPYSIGN y z)) -><br>
(FCOPYSIGN (FCOPYSIGN x y) z)<br>
<br>
[definitely double check me on that one]<br>
<br>
I don't see any other target neutral ones that are like that, but I might be wrong.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
of those ops, then I think we'd hoist it's use even higher so that it's<br>
checked from some common function rather than each potential binop's<br>
visitX() function?<br>
</blockquote>
<br></span>
Seems reasonable to me... especially because it'll have to pull in the Options object to decide whether, for example, FADD is associative.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Is it sufficient for N00:k to be any binop? Can it be relaxed even<br>
further?<br>
<br>
Good point; I missed that generalization. I don't see a problem with this:<br>
x * y + z + w -> (x * y) + (z + w)<br>
<br>
I want to believe we can do it, but I'm not sure yet...particularly if<br>
isCommutativeBinOp() is the full list of potential fold<br>
candidates...because I have no knowledge of some of those opcodes yet. :)<br>
</blockquote>
<br></span>
I'd say, start with a small list of the ones you know for certain are associative, and grow it incrementally from there.<br>
<br>
<br>
Sidenote: if you make that change, then I think it subsumes this transform:<br>
<br>
// fold (fadd (fadd x, c1), c2) -> (fadd x, (fadd c1, c2))<br>
<br>
because it isn't doing any constant folding, and it's the same transform.<br>
<br>
<br>
Cheers,<br>
<br>
Jon<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
<br>
<br>
On Wed, Apr 29, 2015 at 5:16 PM, Jonathan Roelofs<br></span><div><div class="h5">
<<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a> <mailto:<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a>>> wrote:<br>
<br>
<br>
<br>
On 4/29/15 4:30 PM, Sanjay Patel wrote:<br>
<br>
Author: spatel<br>
Date: Wed Apr 29 17:30:02 2015<br>
New Revision: 236171<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=236171&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=236171&view=rev</a><br>
Log:<br>
generalize binop reassociation; NFC<br>
<br>
Move the fold introduced in r236031:<br>
<a href="http://reviews.llvm.org/rL236031" target="_blank">http://reviews.llvm.org/rL236031</a><br>
<br>
to its own helper function, so we can use it for other binops.<br>
<br>
This is a preliminary step before partially solving:<br>
<a href="https://llvm.org/bugs/show_bug.cgi?id=21768" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=21768</a><br>
<a href="https://llvm.org/bugs/show_bug.cgi?id=23116" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=23116</a><br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
<br>
Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=236171&r1=236170&r2=236171&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=236171&r1=236170&r2=236171&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)<br>
+++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Wed Apr<br>
29 17:30:02 2015<br>
@@ -7647,6 +7647,33 @@ SDValue DAGCombiner::visitFSUBForFMAComb<br>
return SDValue();<br>
}<br>
<br>
+static SDValue ReassociateBinops(SDNode *N, SelectionDAG &DAG) {<br>
+ assert(N->getNumOperands() == 2 && "Invalid node for binop<br>
reassociation");<br>
+ SDValue N0 = N->getOperand(0);<br>
+ SDValue N1 = N->getOperand(1);<br>
+ EVT VT = N->getValueType(0);<br>
+ SDLoc DL(N);<br>
+ unsigned Opcode = N->getOpcode();<br>
<br>
<br>
Perhaps add a "the opcode really is associative" assertion?<br>
<br>
+<br>
+ // Canonicalize chains of this operation to LHS to allow the<br>
following fold.<br>
+ if (N0.getOpcode() != Opcode && N1.getOpcode() == Opcode)<br>
+ return DAG.getNode(Opcode, DL, VT, N1, N0);<br>
+<br>
+ // Convert a chain of 3 dependent operations into 2<br>
independent operations<br>
+ // and 1 dependent operation:<br>
+ // (op N0: (op N00: (op z, w), N01: y), N1: x) -><br>
+ // (op N00: (op z, w), (op N1: x, N01: y))<br>
+ if (N0.getOpcode() == Opcode && N0.hasOneUse() &&<br>
N1.getOpcode() != Opcode) {<br>
+ SDValue N00 = N0.getOperand(0);<br>
+ if (N00.getOpcode() == Opcode) {<br>
<br>
<br>
Realizing that your change is NFC...<br>
<br>
Why do we care that N00's opcode matches the other two? N00:k<br>
doesn't really participate in the re-association of N01:y and N1:x:<br>
<br>
(op N0: (op N00: k, N01: y), N1: x) -><br>
(op N00: k, (op N1: x, N01: y))<br>
<br>
or in infix notation without the N's, that is:<br>
<br>
(k + y) + x -><br>
k + (y + x)<br>
<br>
Is it sufficient for N00:k to be any binop? Can it be relaxed even<br>
further?<br>
<br>
<br>
Cheers,<br>
<br>
Jon<br>
<br>
<br>
+ SDValue N01 = N0.getOperand(1);<br>
+ SDValue NewOp = DAG.getNode(Opcode, DL, VT, N1, N01);<br>
+ return DAG.getNode(Opcode, DL, VT, N00, NewOp);<br>
+ }<br>
+ }<br>
+ return SDValue();<br>
+}<br>
+<br>
SDValue DAGCombiner::visitFADD(SDNode *N) {<br>
SDValue N0 = N->getOperand(0);<br>
SDValue N1 = N->getOperand(1);<br>
@@ -7781,24 +7808,10 @@ SDValue DAGCombiner::visitFADD(SDNode *N<br>
N0.getOperand(0),<br>
DAG.getConstantFP(4.0, DL, VT));<br>
}<br>
}<br>
-<br>
- // Canonicalize chains of adds to LHS to simplify the<br>
following transform.<br>
- if (N0.getOpcode() != ISD::FADD && N1.getOpcode() == ISD::FADD)<br>
- return DAG.getNode(ISD::FADD, DL, VT, N1, N0);<br>
<br>
- // Convert a chain of 3 dependent operations into 2<br>
independent operations<br>
- // and 1 dependent operation:<br>
- // (fadd N0: (fadd N00: (fadd z, w), N01: y), N1: x) -><br>
- // (fadd N00: (fadd z, w), (fadd N1: x, N01: y))<br>
- if (N0.getOpcode() == ISD::FADD && N0.hasOneUse() &&<br>
- N1.getOpcode() != ISD::FADD) {<br>
- SDValue N00 = N0.getOperand(0);<br>
- if (N00.getOpcode() == ISD::FADD) {<br>
- SDValue N01 = N0.getOperand(1);<br>
- SDValue NewAdd = DAG.getNode(ISD::FADD, DL, VT, N1, N01);<br>
- return DAG.getNode(ISD::FADD, DL, VT, N00, NewAdd);<br>
- }<br>
- }<br>
+ if (SDValue Reassociated = ReassociateBinops(N, DAG))<br>
+ return Reassociated;<br>
+<br>
} // enable-unsafe-fp-math<br>
<br>
// FADD -> FMA combines:<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br></div></div>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>><span class=""><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
<br>
--<br>
Jon Roelofs<br></span>
<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a> <mailto:<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a>><br>
CodeSourcery / Mentor Embedded<br>
<br>
<br>
</blockquote><div class="HOEnZb"><div class="h5">
<br>
-- <br>
Jon Roelofs<br>
<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a><br>
CodeSourcery / Mentor Embedded<br>
</div></div></blockquote></div><br></div>