This is an archive of the discontinued LLVM Phabricator instance.

Fold fneg and fabs like multiplications
ClosedPublic

Authored by rampitec on Jun 23 2017, 4:06 PM.

Details

Summary

Given no NaNs and no signed zeroes it folds:

(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg (fabs X))
(fmul X, (select (fcmp X > 0.0), 1.0, -1.0)) -> (fabs X)

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec created this revision.Jun 23 2017, 4:06 PM
hfinkel added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9772 ↗(On Diff #103793)

According to the docs in include/llvm/CodeGen/ISDOpcodes.h, for floating point you should handle the SETLT and friends as well (these just mean that it is undefined if the input is NaN).

9776 ↗(On Diff #103793)

Visually, I'd prefer that the std::swap be on its own line below this one. Otherwise, it looks like all of the condition code fall through to the bottom.

rampitec updated this revision to Diff 103801.Jun 23 2017, 4:35 PM
rampitec marked 2 inline comments as done.

Addressed review comments.

(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg X)

Is not correct.

It should be:
(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg (fabs X))

If X was -2.0, (fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) is (fmul -2.0, (select false, -1.0, 1.0) ) =>(fmul -2.0, 1.0 ) => -2.0. Which is different from (fneg -2.0) => 2.0.

rampitec planned changes to this revision.Jun 23 2017, 4:44 PM

(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg X)

Is not correct.

It should be:
(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg (fabs X))

If X was -2.0, (fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) is (fmul -2.0, (select false, -1.0, 1.0) ) =>(fmul -2.0, 1.0 ) => -2.0. Which is different from (fneg -2.0) => 2.0.

Ugh... right.

rampitec updated this revision to Diff 103805.Jun 23 2017, 4:54 PM
rampitec edited the summary of this revision. (Show Details)

Fixed (fneg (fabs X)) case.

arsenm added inline comments.Jun 23 2017, 5:07 PM
test/CodeGen/AMDGPU/fold-fmul-to-neg-abs.ll
34 ↗(On Diff #103805)

Needs test for fneg/fabs case

rampitec added inline comments.Jun 23 2017, 5:10 PM
test/CodeGen/AMDGPU/fold-fmul-to-neg-abs.ll
34 ↗(On Diff #103805)

That is the first test. fneg(fabs) became v_or_b32. Initially it was an incorrect xor.

vpykhtin accepted this revision.Jun 27 2017, 3:12 AM

LGTM.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9767 ↗(On Diff #103805)

I understand what you meant but at first glance it looks like "if ( 2 * 2 == 4 )" :-) Should it be named like TrueOpnd and FalseOpnd?

This revision is now accepted and ready to land.Jun 27 2017, 3:12 AM
rampitec updated this revision to Diff 104208.Jun 27 2017, 10:33 AM
rampitec marked an inline comment as done.

Renamed variables True and False into TrueOpnd and FalseOpnd as suggested.

This revision was automatically updated to reflect the committed changes.

Note why don't you
fold "(select (fcmp X > 0.0), -1.0, 1.0)" into copysign(1.0, X)
fold "(select (fcmp X > 0.0), 1.0, -1.0)" into copysign(1.0, -X)
?

I did that for GCC and then optimized:
X * copysign(1.0, X) into abs(X)
X * copysign(1.0, -X) into -abs(X)

Note why don't you
fold "(select (fcmp X > 0.0), -1.0, 1.0)" into copysign(1.0, X)
fold "(select (fcmp X > 0.0), 1.0, -1.0)" into copysign(1.0, -X)
?

I did that for GCC and then optimized:
X * copysign(1.0, X) into abs(X)
X * copysign(1.0, -X) into -abs(X)

Hm.. That can be an interesting idea too.