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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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)
----------------
cameron.mcinally wrote:
> 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.
That was D50417. 

If there are multiple uses, the 2 potential forms would look something like this:
  T = fdiv X, C
  R = fneg T
vs. 
  T = fdiv X, C
  R = fdiv X,-C

So correct, no difference in instruction count. And I agree that this isn't our usual trade-off in IR/analysis. But I do wonder if the fneg allows for better reassociation opportunities. Not sure how to check that though.

I also agree on the codegen/perf concern, but that would have to be added first to the backend IMO to avoid (the theoretical, but potentially large) regression, so I went with the conservative quick fix. 

I don't think we can get away with an SDAG solution. If those fdivs end up in different basic blocks somehow, we need a global analysis to know the siblings exist.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61693