[llvm] r329821 - [InstCombine] limit X - (cast(-Y) --> X + cast(Y) with hasOneUse()

via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 13 12:46:43 PDT 2018 

i think i’m going absolutely bonkers; i can’t actually find the original 0.14% instruction count regression, despite having a log of tests that shows it. i think i’m losing my mind; reverting this commit doesn’t even undo the “regression” even though it did when i tested it last. nevermind this complaint.

—escha

> On Apr 13, 2018, at 11:57 AM, escha at apple.com wrote:
> 
> this doesn’t seem to help at all, even if I add isfpextfree to our targets. i suspect we’re losing out on a transformation done during the IR stage (e.g. reassociation) as a result of this. i’m going to look into it further.
> 
> —escha
> 
>> On Apr 13, 2018, at 9:40 AM, via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> 
>> okay, will test this, thanks!
>> 
>> —escha
>> 
>>> On Apr 12, 2018, at 3:55 PM, Sanjay Patel <spatel at rotateright.com <mailto:spatel at rotateright.com>> wrote:
>>> 
>>> Patch that might fix the problem proposed here:
>>> https://reviews.llvm.org/D45598 <https://reviews.llvm.org/D45598>
>>> 
>>> On Thu, Apr 12, 2018 at 1:00 PM, Sanjay Patel <spatel at rotateright.com <mailto:spatel at rotateright.com>> wrote:
>>> Hmm...so this one is different than r329350, right?
>>> 
>>> In this case, we really shouldn't canonicalize to a form with more instructions. Add a DAGCombine that does the transform if fpext/fptrunc are free?
>>> 
>>> Let me know if I should revert in the meantime.
>>> 
>>> On Thu, Apr 12, 2018 at 11:57 AM, <escha at apple.com <mailto:escha at apple.com>> wrote:
>>> hi! this patch caused a 0.14% instruction count regression on our test suite. it is likely quite bad for us because fpext/fptrunc are free.
>>> 
>>> —escha
>>> 
>>> > On Apr 11, 2018, at 8:57 AM, Sanjay Patel via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>> > 
>>> > Author: spatel
>>> > Date: Wed Apr 11 08:57:18 2018
>>> > New Revision: 329821
>>> > 
>>> > URL: http://llvm.org/viewvc/llvm-project?rev=329821&view=rev <http://llvm.org/viewvc/llvm-project?rev=329821&view=rev>
>>> > Log:
>>> > [InstCombine] limit X - (cast(-Y) --> X + cast(Y) with hasOneUse()
>>> > 
>>> > Modified:
>>> >    llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> >    llvm/trunk/test/Transforms/InstCombine/fsub.ll
>>> > 
>>> > Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp?rev=329821&r1=329820&r2=329821&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp?rev=329821&r1=329820&r2=329821&view=diff>
>>> > ==============================================================================
>>> > --- llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp (original)
>>> > +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp Wed Apr 11 08:57:18 2018
>>> > @@ -1731,16 +1731,16 @@ Instruction *InstCombiner::visitFSub(Bin
>>> >   if (match(Op1, m_FNeg(m_Value(Y))))
>>> >     return BinaryOperator::CreateFAddFMF(Op0, Y, &I);
>>> > 
>>> > -  if (FPTruncInst *FPTI = dyn_cast<FPTruncInst>(Op1)) {
>>> > -    if (Value *V = dyn_castFNegVal(FPTI->getOperand(0))) {
>>> > -      Value *NewTrunc = Builder.CreateFPTrunc(V, I.getType());
>>> > -      return BinaryOperator::CreateFAddFMF(Op0, NewTrunc, &I);
>>> > -    }
>>> > -  } else if (FPExtInst *FPEI = dyn_cast<FPExtInst>(Op1)) {
>>> > -    if (Value *V = dyn_castFNegVal(FPEI->getOperand(0))) {
>>> > -      Value *NewExt = Builder.CreateFPExt(V, I.getType());
>>> > -      return BinaryOperator::CreateFAddFMF(Op0, NewExt, &I);
>>> > -    }
>>> > +  // Similar to above, but look through a cast of the negated value:
>>> > +  // X - (fptrunc(-Y)) --> X + fptrunc(Y)
>>> > +  if (match(Op1, m_OneUse(m_FPTrunc(m_FNeg(m_Value(Y)))))) {
>>> > +    Value *TruncY = Builder.CreateFPTrunc(Y, I.getType());
>>> > +    return BinaryOperator::CreateFAddFMF(Op0, TruncY, &I);
>>> > +  }
>>> > +  // X - (fpext(-Y)) --> X + fpext(Y)
>>> > +  if (match(Op1, m_OneUse(m_FPExt(m_FNeg(m_Value(Y)))))) {
>>> > +    Value *ExtY = Builder.CreateFPExt(Y, I.getType());
>>> > +    return BinaryOperator::CreateFAddFMF(Op0, ExtY, &I);
>>> >   }
>>> > 
>>> >   // Handle specials cases for FSub with selects feeding the operation
>>> > 
>>> > Modified: llvm/trunk/test/Transforms/InstCombine/fsub.ll
>>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/fsub.ll?rev=329821&r1=329820&r2=329821&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/fsub.ll?rev=329821&r1=329820&r2=329821&view=diff>
>>> > ==============================================================================
>>> > --- llvm/trunk/test/Transforms/InstCombine/fsub.ll (original)
>>> > +++ llvm/trunk/test/Transforms/InstCombine/fsub.ll Wed Apr 11 08:57:18 2018
>>> > @@ -189,14 +189,13 @@ define double @neg_ext_op1_fast(float %a
>>> >   ret double %t3
>>> > }
>>> > 
>>> > -; FIXME: Extra use should prevent the transform.
>>> > +; Extra use should prevent the transform.
>>> > 
>>> > define float @neg_ext_op1_extra_use(half %a, float %b) {
>>> > ; CHECK-LABEL: @neg_ext_op1_extra_use(
>>> > ; CHECK-NEXT:    [[T1:%.*]] = fsub half 0xH8000, [[A:%.*]]
>>> > ; CHECK-NEXT:    [[T2:%.*]] = fpext half [[T1]] to float
>>> > -; CHECK-NEXT:    [[TMP1:%.*]] = fpext half [[A]] to float
>>> > -; CHECK-NEXT:    [[T3:%.*]] = fadd float [[TMP1]], [[B:%.*]]
>>> > +; CHECK-NEXT:    [[T3:%.*]] = fsub float [[B:%.*]], [[T2]]
>>> > ; CHECK-NEXT:    call void @use(float [[T2]])
>>> > ; CHECK-NEXT:    ret float [[T3]]
>>> > ;
>>> > @@ -207,8 +206,8 @@ define float @neg_ext_op1_extra_use(half
>>> >   ret float %t3
>>> > }
>>> > 
>>> > -; One-use fptrunc is always hoisted above fneg, so the corresponding 
>>> > -; multi-use bug for fptrunc isn't visible with a fold starting from 
>>> > +; One-use fptrunc is always hoisted above fneg, so the corresponding
>>> > +; multi-use bug for fptrunc isn't visible with a fold starting from
>>> > ; the last fsub.
>>> > 
>>> > define float @neg_trunc_op1_extra_use(double %a, float %b) {
>>> > @@ -226,15 +225,13 @@ define float @neg_trunc_op1_extra_use(do
>>> >   ret float %t3
>>> > }
>>> > 
>>> > -; FIXME: But the bug is visible when both preceding values have other uses.
>>> > ; Extra uses should prevent the transform.
>>> > 
>>> > define float @neg_trunc_op1_extra_uses(double %a, float %b) {
>>> > ; CHECK-LABEL: @neg_trunc_op1_extra_uses(
>>> > ; CHECK-NEXT:    [[T1:%.*]] = fsub double -0.000000e+00, [[A:%.*]]
>>> > ; CHECK-NEXT:    [[T2:%.*]] = fptrunc double [[T1]] to float
>>> > -; CHECK-NEXT:    [[TMP1:%.*]] = fptrunc double [[A]] to float
>>> > -; CHECK-NEXT:    [[T3:%.*]] = fadd float [[TMP1]], [[B:%.*]]
>>> > +; CHECK-NEXT:    [[T3:%.*]] = fsub float [[B:%.*]], [[T2]]
>>> > ; CHECK-NEXT:    call void @use2(float [[T2]], double [[T1]])
>>> > ; CHECK-NEXT:    ret float [[T3]]
>>> > ;
>>> > 
>>> > 
>>> > _______________________________________________
>>> > llvm-commits mailing list
>>> > llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>> 
>>> 
>>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180413/22c029e4/attachment-0001.html>

More information about the llvm-commits mailing list