This is an archive of the discontinued LLVM Phabricator instance.

[Reassoc] Small fix to support unary FNeg in NegateValue(...)
ClosedPublic

Authored by cameron.mcinally on Aug 22 2019, 11:47 AM.

Details

Summary

Here's a small patch to fix an assert in NegateValue(...).

This may need further performance improvements...

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2019, 11:47 AM

LGTM, but I will give Sanjay some time before moving forward.

llvm/lib/Transforms/Scalar/Reassociate.cpp
916 ↗(On Diff #216680)

You will need to revise this one too in D61675 as it will be a unary then. Just a note to remember.

BTW, general comment: Many of the remaining BinaryOperator contexts in Reassociate.cpp will need re-examaination too as UnaryOperator may also be present in places where it currently is not.

cameron.mcinally marked an inline comment as done.Aug 22 2019, 1:25 PM
cameron.mcinally added inline comments.
llvm/lib/Transforms/Scalar/Reassociate.cpp
916 ↗(On Diff #216680)

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

BTW, general comment: Many of the remaining BinaryOperator contexts in Reassociate.cpp will need re-examaination too as UnaryOperator may also be present in places where it currently is not.

Agreed.

If you have any test cases that I can use as an intuition pump, that would be great. I haven't worked that much in Reassociate.

spatel accepted this revision.Aug 22 2019, 2:46 PM

LGTM

llvm/lib/Transforms/Scalar/Reassociate.cpp
916 ↗(On Diff #216680)

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.

llvm/test/Transforms/Reassociate/2019-08-22-FNegAssert.ll
1 ↗(On Diff #216680)

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.

This revision is now accepted and ready to land.Aug 22 2019, 2:46 PM
mcberg2017 accepted this revision.Aug 22 2019, 3:24 PM
This revision was automatically updated to reflect the committed changes.