<div dir="ltr"><div><div><div><div><div>Hi Jon -<br><br></div>Thanks for the review!<br></div><br>> Perhaps add a "the opcode really is associative" assertion?<span class=""><br></span><br>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?<br><br>> Is it sufficient for N00:k to be any binop? Can it be relaxed even further?<br><br></div>Good point; I missed that generalization. I don't see a problem with this:<br></div>x * y + z + w -> (x * y) + (z + w)<br><br></div>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. :)<br><div><br><div><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 29, 2015 at 5:16 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5"><br>
<br>
On 4/29/15 4:30 PM, Sanjay Patel wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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: <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 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 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>
</blockquote>
<br></div></div>
Perhaps add a "the opcode really is associative" assertion?<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ // Canonicalize chains of this operation to LHS to allow the 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 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() && N1.getOpcode() != Opcode) {<br>
+ SDValue N00 = N0.getOperand(0);<br>
+ if (N00.getOpcode() == Opcode) {<br>
</blockquote>
<br></span>
Realizing that your change is NFC...<br>
<br>
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:<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 further?<br>
<br>
<br>
Cheers,<br>
<br>
Jon<div class=""><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ 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), DAG.getConstantFP(4.0, DL, VT));<br>
}<br>
}<br>
-<br>
- // Canonicalize chains of adds to LHS to simplify the 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 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>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><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>
</blockquote>
<br></div></div><span class=""><font color="#888888">
-- <br>
Jon Roelofs<br>
<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a><br>
CodeSourcery / Mentor Embedded<br>
</font></span></blockquote></div><br></div></div></div></div></div>