<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Apr 30, 2015, at 8:19 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" class="">spatel@rotateright.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div class=""><div class=""><div class="">> Also, isn’t there the exact same register pressure concern in the
DAG as well? Especially considering the fact that this is
target-independent?<br class=""><br class=""></div>Now that the patch is reverted due to a spilling catastrophe, this seems obvious. :)<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>I don’t know, you may see other reasons why it can even be worse in InstCombine :)</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class=""><br class=""></div>I also seem to have completely misjudged the potential compile-time hit. I did look at that using test-suite on x86, so I'd love to know if D9363 made any difference for your out-of-tree target and/or tests.<br class=""></div></div></div></blockquote><div><br class=""></div><div>Well it solves the infinite loop we had locally at least :)</div><div><br class=""></div><div>I’m still using bugpoint on the test that goes from 6338 to 18559 instructions locally when applying your change (with D9363).</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""><br class=""><div class="">> By the way the name of the function is very generic
(ReassociateBinops), do you plan to extend it to do perform
transformations?<br class="">> Right now it does not carry what it does.</div><span class=""><div class=""><br class=""></div></span></div>I had a vague feeling that someone, someday would extend it in some interesting way, but the truth is really just that I don't know what to name this:<br class=""><br class="">// (op N0: (op N00: (op z, w), N01: y), N1: x) -><br class="">
// (op N00: (op z, w), (op N1: x, N01: y))<br class=""></div></div></blockquote><div><br class=""></div><div>I don’t have a great suggestion.</div><div><br class=""></div><div>— </div><div>Mehdi</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">
<br class=""><div class=""><div class=""><br class=""><br class=""><div class=""><div class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Wed, Apr 29, 2015 at 10:03 PM, Mehdi Amini <span dir="ltr" class=""><<a href="mailto:mehdi.amini@apple.com" target="_blank" class="">mehdi.amini@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word" class="">Hi Sanjay, <div class=""><br class=""></div><div class="">Thanks for your detailed answer.</div><div class=""><br class=""></div><div class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Apr 29, 2015, at 8:03 PM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank" class="">spatel@rotateright.com</a>> wrote:</div><br class=""><div class="">Hi Mehdi,<div class=""><br class=""></div><div class="">Let me answer the last question first: why not instcombine?</div><div class="">Because I noticed this pattern was created by another DAGCombine in:</div><div class=""><font size="2" class=""><span style="background-color:rgba(255,255,255,0)" class=""><a href="http://reviews.llvm.org/D8941" target="_blank" class="">http://reviews.llvm.org/D8941</a></span></font><br class=""><br class="">So although doing the fold here does not preclude an instcombine, doing an instcombine alone would not be sufficient. </div></div></blockquote><div class=""><br class=""></div></span><div class="">Good point!</div><span class=""><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class="">I'm also not sure that the transform should be done that early because it's not a completely free transform; I think there is a potential register pressure concern.</div></div></blockquote><div class=""><br class=""></div><div class=""><br class=""></div></span><div class="">How does the fact that we do it early would increase the register pressure? </div><div class="">Also, isn’t there the exact same register pressure concern in the DAG as well? Especially considering the fact that this is target-independent?</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""></div><div class="">I agree that the fold should be applied more generally. Please see the follow up patch and post commit thread for r236171. I welcome any advice on how to do this better.</div></div></blockquote><div class=""><br class=""></div></span><div class="">I gave a try in <a href="http://reviews.llvm.org/D9363" target="_blank" class="">http://reviews.llvm.org/D9363</a> ; let me know what you think.</div><div class="">By the way the name of the function is very generic (ReassociateBinops), do you plan to extend it to do perform transformations? Right now it does not carry what it does.</div><span class=""><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""></div><div class="">Regarding the canonicalization, I wasn't aware of any downside to commuting the operands, so yes, I added it to not have to duplicate the fold logic for all permutations of the operand order. I was also inf-looping in an early draft of the patch, and canonicalization was my solution to avoid that without multiplying the if checks. If these are not valid reasons for the canonicalization, I can fix it.</div></div></blockquote><div class=""><br class=""></div></span><div class="">This is introducing a strong canonicalization which is not very constrained. No target would be able to canonicalize on more specific conditions. </div><div class="">I don’t know people use to judge if a canonicalization is justified or not, I tend to at least look at a “cost/benefit” analysis: </div><div class="">- how does this enable/simplify other transformations?</div><div class="">- how does this constrains other transformations (infinite loop problem…)</div><div class=""><br class=""></div><div class="">The benefits are unclear to me right now, but that does not mean they don’t exist.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">— </div><span class=""><font color="#888888" class=""><div class="">Mehdi</div></font></span><div class=""><div class=""><div class=""><br class=""></div><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><br class="">On Wednesday, April 29, 2015, Mehdi Amini <<a class="">mehdi.amini@apple.com</a>> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello Sanjay,<br class="">
<br class="">
In this commit you introduce a new canonicalization in the DAG:<br class="">
<br class="">
(fadd (otherop, fadd)) -> (fadd (fadd, otherop))<br class="">
<br class="">
It is not clear to me why this canonicalization is desirable in general (other than to ease the implementation of your transformation, which does not seem to justify it alone IMO).<br class="">
<br class="">
Also, if such a canonicalization is needed, my feeling is that it shouldn’t be hidden deeply here, and should probably apply to a bunch of other binop.<br class="">
<br class="">
As a side question: I’m curious why isn’t this been performed in instcombine?<br class="">
<br class="">
Thanks,<br class="">
<br class="">
Mehdi<br class="">
<br class="">
<br class="">
<br class="">
> On Apr 28, 2015, at 2:03 PM, Sanjay Patel <<a class="">spatel@rotateright.com</a>> wrote:<br class="">
><br class="">
> Author: spatel<br class="">
> Date: Tue Apr 28 16:03:22 2015<br class="">
> New Revision: 236031<br class="">
><br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=236031&view=rev" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=236031&view=rev</a><br class="">
> Log:<br class="">
> transform fadd chains to increase parallelism<br class="">
><br class="">
> This is a compromise: with this simple patch, we should always handle a chain of exactly 3<br class="">
> operations optimally, but we're not generating the optimal balanced binary tree for a longer<br class="">
> sequence.<br class="">
><br class="">
> In general, this transform will reduce the dependency chain for a sequence of instructions<br class="">
> using N operands from a worst case N-1 dependent operations to N/2 dependent operations.<br class="">
> The optimal balanced binary tree would reduce the chain to log2(N).<br class="">
><br class="">
> The trade-off for not dealing with longer sequences is: (1) we have less complexity in the<br class="">
> compiler, (2) we avoid unknown compile-time blowup calculating a balanced tree, and (3) we<br class="">
> don't need to worry about the increased register pressure required to parallelize longer<br class="">
> sequences. It also seems unlikely that we would ever encounter really long strings of<br class="">
> dependent ops like that in the wild, but I'm not sure how to verify that speculation.<br class="">
> FWIW, I see no perf difference for test-suite running on btver2 (x86-64) with -ffast-math<br class="">
> and this patch.<br class="">
><br class="">
> We can extend this patch to cover other associative operations such as fmul, fmax, fmin,<br class="">
> integer add, integer mul.<br class="">
><br class="">
> This is a partial fix for:<br class="">
> <a href="https://llvm.org/bugs/show_bug.cgi?id=17305" target="_blank" class="">https://llvm.org/bugs/show_bug.cgi?id=17305</a><br class="">
><br class="">
> and if extended:<br class="">
> <a href="https://llvm.org/bugs/show_bug.cgi?id=21768" target="_blank" class="">https://llvm.org/bugs/show_bug.cgi?id=21768</a><br class="">
> <a href="https://llvm.org/bugs/show_bug.cgi?id=23116" target="_blank" class="">https://llvm.org/bugs/show_bug.cgi?id=23116</a><br class="">
><br class="">
> The issue also came up in:<br class="">
> <a href="http://reviews.llvm.org/D8941" target="_blank" class="">http://reviews.llvm.org/D8941</a><br class="">
><br class="">
> Differential Revision: <a href="http://reviews.llvm.org/D9232" target="_blank" class="">http://reviews.llvm.org/D9232</a><br class="">
><br class="">
><br class="">
> Modified:<br class="">
> llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br class="">
> llvm/trunk/test/CodeGen/X86/fp-fast.ll<br class="">
><br class="">
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=236031&r1=236030&r2=236031&view=diff" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=236031&r1=236030&r2=236031&view=diff</a><br class="">
> ==============================================================================<br class="">
> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)<br class="">
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Tue Apr 28 16:03:22 2015<br class="">
> @@ -7801,6 +7801,24 @@ SDValue DAGCombiner::visitFADD(SDNode *N<br class="">
> N0.getOperand(0), DAG.getConstantFP(4.0, DL, VT));<br class="">
> }<br class="">
> }<br class="">
> +<br class="">
> + // Canonicalize chains of adds to LHS to simplify the following transform.<br class="">
> + if (N0.getOpcode() != ISD::FADD && N1.getOpcode() == ISD::FADD)<br class="">
> + return DAG.getNode(ISD::FADD, SDLoc(N), VT, N1, N0);<br class="">
> +<br class="">
> + // Convert a chain of 3 dependent operations into 2 independent operations<br class="">
> + // and 1 dependent operation:<br class="">
> + // (fadd N0: (fadd N00: (fadd z, w), N01: y), N1: x) -><br class="">
> + // (fadd N00: (fadd z, w), (fadd N1: x, N01: y))<br class="">
> + if (N0.getOpcode() == ISD::FADD && N0.hasOneUse() &&<br class="">
> + N1.getOpcode() != ISD::FADD) {<br class="">
> + SDValue N00 = N0.getOperand(0);<br class="">
> + if (N00.getOpcode() == ISD::FADD) {<br class="">
> + SDValue N01 = N0.getOperand(1);<br class="">
> + SDValue NewAdd = DAG.getNode(ISD::FADD, SDLoc(N), VT, N1, N01);<br class="">
> + return DAG.getNode(ISD::FADD, SDLoc(N), VT, N00, NewAdd);<br class="">
> + }<br class="">
> + }<br class="">
> } // enable-unsafe-fp-math<br class="">
><br class="">
> // FADD -> FMA combines:<br class="">
><br class="">
> Modified: llvm/trunk/test/CodeGen/X86/fp-fast.ll<br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fp-fast.ll?rev=236031&r1=236030&r2=236031&view=diff" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fp-fast.ll?rev=236031&r1=236030&r2=236031&view=diff</a><br class="">
> ==============================================================================<br class="">
> --- llvm/trunk/test/CodeGen/X86/fp-fast.ll (original)<br class="">
> +++ llvm/trunk/test/CodeGen/X86/fp-fast.ll Tue Apr 28 16:03:22 2015<br class="">
> @@ -113,3 +113,46 @@ define float @test11(float %a) {<br class="">
> %t2 = fadd float %a, %t1<br class="">
> ret float %t2<br class="">
> }<br class="">
> +<br class="">
> +; Verify that the first two adds are independent; the destination registers<br class="">
> +; are used as source registers for the third add.<br class="">
> +<br class="">
> +define float @reassociate_adds1(float %a, float %b, float %c, float %d) {<br class="">
> +; CHECK-LABEL: reassociate_adds1:<br class="">
> +; CHECK: # BB#0:<br class="">
> +; CHECK-NEXT: vaddss %xmm1, %xmm0, %xmm0<br class="">
> +; CHECK-NEXT: vaddss %xmm2, %xmm3, %xmm1<br class="">
> +; CHECK-NEXT: vaddss %xmm1, %xmm0, %xmm0<br class="">
> +; CHECK-NEXT: retq<br class="">
> + %add0 = fadd float %a, %b<br class="">
> + %add1 = fadd float %add0, %c<br class="">
> + %add2 = fadd float %add1, %d<br class="">
> + ret float %add2<br class="">
> +}<br class="">
> +<br class="">
> +define float @reassociate_adds2(float %a, float %b, float %c, float %d) {<br class="">
> +; CHECK-LABEL: reassociate_adds2:<br class="">
> +; CHECK: # BB#0:<br class="">
> +; CHECK-NEXT: vaddss %xmm1, %xmm0, %xmm0<br class="">
> +; CHECK-NEXT: vaddss %xmm2, %xmm3, %xmm1<br class="">
> +; CHECK-NEXT: vaddss %xmm1, %xmm0, %xmm0<br class="">
> +; CHECK-NEXT: retq<br class="">
> + %add0 = fadd float %a, %b<br class="">
> + %add1 = fadd float %c, %add0<br class="">
> + %add2 = fadd float %add1, %d<br class="">
> + ret float %add2<br class="">
> +}<br class="">
> +<br class="">
> +define float @reassociate_adds3(float %a, float %b, float %c, float %d) {<br class="">
> +; CHECK-LABEL: reassociate_adds3:<br class="">
> +; CHECK: # BB#0:<br class="">
> +; CHECK-NEXT: vaddss %xmm1, %xmm0, %xmm0<br class="">
> +; CHECK-NEXT: vaddss %xmm2, %xmm3, %xmm1<br class="">
> +; CHECK-NEXT: vaddss %xmm1, %xmm0, %xmm0<br class="">
> +; CHECK-NEXT: retq<br class="">
> + %add0 = fadd float %a, %b<br class="">
> + %add1 = fadd float %add0, %c<br class="">
> + %add2 = fadd float %d, %add1<br class="">
> + ret float %add2<br class="">
> +}<br class="">
> +<br class="">
><br class="">
><br class="">
> _______________________________________________<br class="">
> llvm-commits mailing list<br class="">
> <a class="">llvm-commits@cs.uiuc.edu</a><br class="">
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class="">
<br class="">
</blockquote></div>
</div></blockquote></div></div></div><br class=""></div></div></blockquote></div><br class=""></div></div></div></div></div></div>
</div></blockquote></div><br class=""></body></html>