[PATCH] D61675: [WIP] Update IRBuilder::CreateFNeg(...) to return a UnaryOperator

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D61675#1670602 <https://reviews.llvm.org/D61675#1670602>, @spatel wrote:

> In D61675#1663625 <https://reviews.llvm.org/D61675#1663625>, @cameron.mcinally wrote:
>
> > Ping.
> >
> > I've been digging through this pass and it seems to be ok AFAICT.  OptimizeInst(...) canonicalizes both unary and binary FNegs to -1.0*X, if they are fast and part of a special multiply tree. Other FNegs end up as leaf nodes, so no problem there.
> >
> > Anyone aware of other situations I should look at?
>
>
> Do we need to enhance EarlyCSE to see this equivalence:
>
>   define float @cse_fneg(float %x, i1 %cond) {
>     %fneg_unary = fneg float %x
>     %fneg_binary = fsub float -0.0, %x
>     %r = select i1 %cond, float %fneg_unary, float %fneg_binary
>     ret float %r
>   }
>   
>
> The binary fneg has the looser requirement for NaN propagation (IEEE-754 6.3: "this standard does not specify the sign bit of a NaN result [for math ops]"), but that's less important for optimization than knowing the 2 values are otherwise equivalent.


On 2nd thought, that doesn't really make sense for CSE. The real question is whether we should canonicalize the binary fneg to unary fneg in instcombine. And should that happen before/after/concurrent with this change to clang?


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

https://reviews.llvm.org/D61675