[PATCH] D61693: Rough out InstCombine::visitFNeg(...)

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked an inline comment as done.
cameron.mcinally added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1828-1829
+
+  // Fold negation into constant operand. This is limited with one-use because
+  // fneg is assumed better for analysis and cheaper in codegen than fmul/fdiv.
+  // -(X * C) --> X * (-C)
----------------
lebedev.ri wrote:
> I'm not confident this one-use check should be here.
> We start with two ops - negation and some inner operand.
> We will drop the negation, replace with with a single op we build.
> Therefore there is no instruction count explosion.
> And less instructions should be pretty much always better.
> 
> Though i see that this is only moving it around.
> 
> The performance concern (the inverse transform,
> `X * (-C)` -> `-(X * C)` if `(X * C)` already exists)
> should be a separate fold in dagcombine..
>I'm not confident this one-use check should be here.
>We start with two ops - negation and some inner operand.
>We will drop the negation, replace with with a single op we build.
>Therefore there is no instruction count explosion.
>And less instructions should be pretty much always better.

Depends on the architecture really. Worst case, we could end up with a NEG and a MUL, or alternatively 2 MUL's. If a MUL is slower than an XOR (the NEG), then the check would make sense.

> The performance concern (the inverse transform,
>X * (-C) -> -(X * C) if (X * C) already exists)
>should be a separate fold in dagcombine..

That's a good peep. Should probably be handled in a separate Diff though. I'm trying not to modify code here, just replicate FSub xforms for the new FNeg.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61693