[PATCH] D66612: [Reassoc] Small fix to support unary FNeg in NegateValue(...)

[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/Transforms/Scalar/Reassociate.cpp:916
   // negation.
   BinaryOperator *NewNeg = CreateNeg(V, V->getName() + ".neg", BI, BI);
   ToRedo.insert(NewNeg);
----------------
cameron.mcinally wrote:
> mcberg2017 wrote:
> > You will need to revise this one too in D61675 as it will be a unary then.  Just a note to remember.
> I think this one is okay. This is creating a new binary FNeg.
> 
> I'm still working out the details, but I suspect that allowing Reassociation to form binary FNegs is good (i.e. better trees). E.g.:
> 
> x + -y
> 
> can be safely transformed to:
> 
> x + (-0.0 - y)
> 
> as long as the x isn't optimized away (Sanjay showed this in another Diff). I can elaborate if anyone is interested...
For reference, the reasoning is that if 'y' is NaN, and it's subsequently used by the fadd, we aren't required to preserve the resulting signbit of that NaN through the fadd op.


================
Comment at: llvm/test/Transforms/Reassociate/2019-08-22-FNegAssert.ll:1
+; RUN: opt < %s -reassociate -disable-output
+; D61675
----------------
I think it's better to use the usual script here and generate the expected output. In other words, if we're going to test this code for failure, we might as well test it for correctness at the same time. 

The extra checking may also reveal unintended behavior for future patches sooner rather than later.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66612