[PATCH] D62077: [InstSimplify] Teach fsub -0.0, (fneg X) ==> X about unary fneg

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4402
     return X;
 
   // fsub 0.0, (fsub 0.0, X) ==> X if signed zeros are ignored.
----------------
cameron.mcinally wrote:
> I wasn't sure whether to go with the code above or explicitly match both FNeg and FSub. I.e.
> 
> ```
>   // fsub -0.0, (fsub -0.0, X) ==> X
>   // fsub -0.0, (fneg X) ==> X
>   Value *X;
>   if (match(Op0, m_NegZeroFP()) &&
>       (match(Op1, m_FSub(m_NegZeroFP(), m_Value(X))) ||
>        match(Op1, m_FNeg(m_Value(X)))))
>     return X;
> ```
> 
> An FSub(-0.0, X) --> FNeg(X) is ok -- and we should probably match them as such, as we do now. But a purist in the future might want to separate the two. 
> 
> Either option is safe to do, since tests will catch the purist's future change to m_FNeg(...). So it's really just a community preference.
The current patch seems fine to me; as you said, the tests future-proof changes to the underlying matcher.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4405
   // fsub 0.0, (fneg X) ==> X if signed zeros are ignored.
   if (FMF.noSignedZeros() && match(Op0, m_AnyZeroFP()) &&
       (match(Op1, m_FSub(m_AnyZeroFP(), m_Value(X))) ||
----------------
Semi-independent of this patch, but might want to change that m_AnyZeroFP() to m_ZeroFP() now?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62077/new/

https://reviews.llvm.org/D62077