<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Apr 6, 2018, at 9:48 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="">Ah, thanks for letting me know. I did consider that case, but I was in an x86 mindset (and IIRC, your target has free or close-to-free fneg?), so the asm diff looks something like this:<br class=""><br class="">define float @neg_sub_nsz_extra_use(float %x, float %y) {<br class=""> %t1 = fsub float %x, %y<br class=""> %t2 = fsub nsz float -0.0, %t1<br class=""> %t3 = fdiv float %t1, %t2 ; just for illustration of extra use of the 1st fsub...<br class=""> ret float %t3<br class="">}<br class=""><br class=""></div>With fneg:<br class=""> vsubss %xmm1, %xmm0, %xmm0<br class=""> vxorps LCPI0_0(%rip), %xmm0, %xmm1 ; load a constant to fneg...sigh<br class=""> vdivss %xmm1, %xmm0, %xmm0<br class=""><br class=""></div>Replace with fsub:<br class=""><div class=""><div class=""> vsubss %xmm1, %xmm0, %xmm2<br class=""> vsubss %xmm0, %xmm1, %xmm0<br class=""> vdivss %xmm0, %xmm2, %xmm0<br class=""></div></div></div></div></blockquote><div><br class=""></div>Yeah, we have free fneg (as do all GPUs that I know of, including AMDGPU).</div><div><br class=""></div><div>Even on x86, I’m nervous about avoiding a xorps by that method in general; i’d expect xorps to be faster because it uses a different execution pipe, at least in some cases.</div><div><br class=""></div><div>This transform, if done at all, should probably be done in target-specific DAG code, since it’s only profitable on a fairly specific subset of machines.</div><div><br class=""></div><div>I also haven’t looked, but can you double check that your <span style="font-family: Menlo; font-size: 11px; background-color: rgb(255, 255, 255);" class="">Z - (X - Y) --> Z + (Y - X) </span>transform also checks oneuse?</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class=""><br class=""></div><div class="">So let me add the use check (I added the test already at <a href="https://reviews.llvm.org/rL329418" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/<wbr class="">rL329418</a> ) to avoid the problem immediately. But maybe I should look at later target-specific (DAG) transforms that can change one to the other based on prefs?<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>I’m not sure. The category of CPUs where “fsub is faster than xor” seems like a fairly Weird, Special Category. maybe just put it in the x86 backend alone?</div><div><br class=""></div><div>—escha</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Apr 6, 2018 at 9:33 AM, <span dir="ltr" class=""><<a href="mailto:escha@apple.com" target="_blank" class="">escha@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This patch is giving us fairly significant instruction count regressions on a number of shaders. The regressions go away if we gate it with hasOneUse, with makes sense; the patch as committed is duplicating arithmetic! Is it possible to add hasOneUse checks to this? I don’t feel like it’s reasonable to duplicate fsubs just because they have a negated use, especially since the compiler as a whole is very bad at deduplicating them later.<br class="">
<span class="m_-197465863396471569HOEnZb"><font color="#888888" class=""><br class="">
—escha<br class="">
</font></span><div class="m_-197465863396471569HOEnZb"><div class="m_-197465863396471569h5"><br class="">
> On Apr 5, 2018, at 2:37 PM, Sanjay Patel via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class="">
><br class="">
> Author: spatel<br class="">
> Date: Thu Apr 5 14:37:17 2018<br class="">
> New Revision: 329350<br class="">
><br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=329350&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-pr<wbr class="">oject?rev=329350&view=rev</a><br class="">
> Log:<br class="">
> [InstCombine] nsz: -(X - Y) --> Y - X<br class="">
><br class="">
> This restores part of the fold that was removed with rL73243 (PR4374).<br class="">
><br class="">
> Modified:<br class="">
> llvm/trunk/lib/Transforms/Inst<wbr class="">Combine/InstCombineAddSub.cpp<br class="">
> llvm/trunk/test/Transforms/Ins<wbr class="">tCombine/fsub.ll<br class="">
><br class="">
> Modified: llvm/trunk/lib/Transforms/Inst<wbr class="">Combine/InstCombineAddSub.cpp<br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp?rev=329350&r1=329349&r2=329350&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-pr<wbr class="">oject/llvm/trunk/lib/Transform<wbr class="">s/InstCombine/InstCombineAddSu<wbr class="">b.cpp?rev=329350&r1=329349&r2=<wbr class="">329350&view=diff</a><br class="">
> ==============================<wbr class="">==============================<wbr class="">==================<br class="">
> --- llvm/trunk/lib/Transforms/Inst<wbr class="">Combine/InstCombineAddSub.cpp (original)<br class="">
> +++ llvm/trunk/lib/Transforms/Inst<wbr class="">Combine/InstCombineAddSub.cpp Thu Apr 5 14:37:17 2018<br class="">
> @@ -1698,10 +1698,17 @@ Instruction *InstCombiner::visitFSub(Bin<br class="">
> SQ.getWithInstruction(&I)))<br class="">
> return replaceInstUsesWith(I, V);<br class="">
><br class="">
> - // Subtraction from -0.0 is the canonical form of fneg.<br class="">
> - // fsub nsz 0, X ==> fsub nsz -0.0, X<br class="">
> - if (I.getFastMathFlags().noSigned<wbr class="">Zeros() && match(Op0, m_PosZeroFP()))<br class="">
> - return BinaryOperator::CreateFNegFMF(<wbr class="">Op1, &I);<br class="">
> + if (I.hasNoSignedZeros()) {<br class="">
> + // Subtraction from -0.0 is the canonical form of fneg.<br class="">
> + // fsub nsz 0, X ==> fsub nsz -0.0, X<br class="">
> + if (match(Op0, m_PosZeroFP()))<br class="">
> + return BinaryOperator::CreateFNegFMF(<wbr class="">Op1, &I);<br class="">
> +<br class="">
> + // With no-signed-zeros: -(X - Y) --> Y - X<br class="">
> + Value *X, *Y;<br class="">
> + if (match(Op0, m_NegZeroFP()) && match(Op1, m_FSub(m_Value(X), m_Value(Y))))<br class="">
> + return BinaryOperator::CreateFSubFMF(<wbr class="">Y, X, &I);<br class="">
> + }<br class="">
><br class="">
> if (isa<Constant>(Op0))<br class="">
> if (SelectInst *SI = dyn_cast<SelectInst>(Op1))<br class="">
><br class="">
> Modified: llvm/trunk/test/Transforms/Ins<wbr class="">tCombine/fsub.ll<br class="">
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/fsub.ll?rev=329350&r1=329349&r2=329350&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-pr<wbr class="">oject/llvm/trunk/test/Transfor<wbr class="">ms/InstCombine/fsub.ll?rev=<wbr class="">329350&r1=329349&r2=329350&<wbr class="">view=diff</a><br class="">
> ==============================<wbr class="">==============================<wbr class="">==================<br class="">
> --- llvm/trunk/test/Transforms/Ins<wbr class="">tCombine/fsub.ll (original)<br class="">
> +++ llvm/trunk/test/Transforms/Ins<wbr class="">tCombine/fsub.ll Thu Apr 5 14:37:17 2018<br class="">
> @@ -19,8 +19,7 @@ define float @test1(float %x, float %y)<br class="">
><br class="">
> define float @neg_sub(float %x, float %y) {<br class="">
> ; CHECK-LABEL: @neg_sub(<br class="">
> -; CHECK-NEXT: [[T1:%.*]] = fsub float [[X:%.*]], [[Y:%.*]]<br class="">
> -; CHECK-NEXT: [[T2:%.*]] = fsub nsz float -0.000000e+00, [[T1]]<br class="">
> +; CHECK-NEXT: [[T2:%.*]] = fsub nsz float [[Y:%.*]], [[X:%.*]]<br class="">
> ; CHECK-NEXT: ret float [[T2]]<br class="">
> ;<br class="">
> %t1 = fsub float %x, %y<br class="">
><br class="">
><br class="">
> ______________________________<wbr class="">_________________<br class="">
> llvm-commits mailing list<br class="">
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class="">
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/<wbr class="">mailman/listinfo/llvm-commits</a><br class="">
<br class="">
</div></div></blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>