This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] x /c fabs(x) -> copysign(1.0, x)
ClosedPublic

Authored by xbolva00 on Aug 7 2019, 12:38 PM.

Details

Diff Detail

Event Timeline

xbolva00 created this revision.Aug 7 2019, 12:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2019, 12:38 PM
xbolva00 updated this revision to Diff 213971.Aug 7 2019, 12:40 PM

vector tests?

xbolva00 updated this revision to Diff 214218.Aug 8 2019, 1:17 PM

Added vector test

spatel added inline comments.Aug 9 2019, 6:16 AM
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1237

Do we want to handle the commuted pattern too?
fabs(X) / X

1240

Do we need the one-use restriction?

test/Transforms/InstCombine/fabs-copysign.ll
43

This is supposed to be an extra use test, but there's no extra use?

xbolva00 updated this revision to Diff 214378.Aug 9 2019, 7:34 AM
xbolva00 retitled this revision from [InstCombine] x / fabs(x) -> copysign(1.0, x) to [InstCombine] x /c fabs(x) -> copysign(1.0, x).
xbolva00 edited the summary of this revision. (Show Details)

Addressed review notes

xbolva00 marked 4 inline comments as done.Aug 9 2019, 7:35 AM
xbolva00 added inline comments.
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1240

Dropped.

xbolva00 marked 2 inline comments as done.Aug 9 2019, 7:37 AM
xbolva00 added inline comments.
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1246

I tried

match(&I, m_FDiv(m_Value(X), m_Intrinsic<Intrinsic::fabs>(m_Specific(X)))) and it failed to match.. uh.

(this blocked me to possibly introduce m_c_FDiv)

spatel added inline comments.
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1246

You need to use m_Deferred() for that case when matching commutatively (cc @lebedev.ri ).
But I don't think it's worth adding a commutative matcher for fdiv since it isn't a commutative operation. Not too many cases like this where it can be used?

test/Transforms/InstCombine/fabs-copysign.ll
22–30

Please push the new tests with baseline CHECKs as a preliminary step.

xbolva00 updated this revision to Diff 214383.Aug 9 2019, 8:52 AM

Use m_Deferred

xbolva00 marked an inline comment as done.Aug 9 2019, 8:52 AM
xbolva00 added inline comments.
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1246

Thanks!

(and agree)

xbolva00 marked an inline comment as done.Aug 9 2019, 8:53 AM
spatel accepted this revision.Aug 12 2019, 6:16 AM

LGTM

This revision is now accepted and ready to land.Aug 12 2019, 6:16 AM

Thanks

Wait - there was a vector test here at 1 point, but now it's gone? Please adjust the type on one of the positive tests, so we still have that coverage.

Uh, that’s interesting how that test was lost.

I will add it again, sorry.

xbolva00 updated this revision to Diff 214626.Aug 12 2019, 6:40 AM

Readded vector tests

This revision was automatically updated to reflect the committed changes.