Page MenuHomePhabricator

Rough out InstCombine::visitFNeg(...)
ClosedPublic

Authored by cameron.mcinally on May 8 2019, 12:27 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 12:27 PM
lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1828–1829 ↗(On Diff #198711)

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..

cameron.mcinally marked an inline comment as done.May 8 2019, 1:00 PM
cameron.mcinally added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1828–1829 ↗(On Diff #198711)

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.

spatel added inline comments.May 8 2019, 1:17 PM
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1828–1829 ↗(On Diff #198711)

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.

Rename CreateWithCopiedFlags(...) argument, since it is no longer strictly a BinaryOperator.

Rebase patch. It's a little more palatable after merging D61784.

cameron.mcinally marked 2 inline comments as done.May 10 2019, 1:15 PM

Ping.

Are we okay with addressing the concerns @lebedev.ri brought up in a different patch? I could file a bug if wanted.

Ping.

LGTM

Are we okay with addressing the concerns @lebedev.ri brought up in a different patch? I could file a bug if wanted.

It's definitely a different patch (if we can actually do anything about it). Adding details from the discussion here to the existing code comment would be good as a separate NFC too.

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1824 ↗(On Diff #199068)

Would be nice to have a documentation comment for this function that says something like:
This eliminates floating-point negation in either 'fneg' or 'fsub' form by combining into a constant operand.

spatel accepted this revision.May 20 2019, 9:52 AM
This revision is now accepted and ready to land.May 20 2019, 9:52 AM
This revision was automatically updated to reflect the committed changes.

Bah, I forgot to amend my commit message before committing. For posterity's sake, it should have read:

[InstCombine] Create foldFNegIntoConstant(...) helper for FNeg

Break out a helper function, namely foldFNegIntoConstant(...), which performs transforms common between visitFNeg(...) and visitFSub(...).

Differential Revision: https://reviews.llvm.org/D61693
cameron.mcinally marked an inline comment as done.May 20 2019, 12:25 PM