This is an archive of the discontinued LLVM Phabricator instance.

InstSimplify: Eliminate fabs on known positive
ClosedPublic

Authored by arsenm on Dec 19 2016, 11:18 AM.

Details

Reviewers
scanon
efriedma

Diff Detail

Event Timeline

arsenm updated this revision to Diff 81978.Dec 19 2016, 11:18 AM
arsenm retitled this revision from to InstSimplify: Eliminate fabs on known positive.
arsenm updated this object.
arsenm added a subscriber: llvm-commits.

SimplifyIntrinsic() is structured in a confusing way; can you rearrange the whole function (in a separate patch) to look more like the following?

if (IsIdempotent(IID)) {
  [...]
}
switch (IID) {
case Intrinsic::usub_with_overflow:
[...]
case Intrinsic::fabs:
[...]
}
test/Transforms/InstSimplify/floating-point-arithmetic.ll
133–134

fabs() returns +0 if the input is -0, so we can't eliminate it here.

efriedma requested changes to this revision.Dec 19 2016, 11:59 AM
efriedma added a reviewer: efriedma.
This revision now requires changes to proceed.Dec 19 2016, 11:59 AM
arsenm updated this revision to Diff 81983.Dec 19 2016, 12:16 PM
arsenm edited edge metadata.

Fix broken -0 handling

efriedma added inline comments.Dec 19 2016, 1:31 PM
lib/Analysis/ValueTracking.cpp
2585

Maybe explicitly handle NaNs here to make it more clear?

2612–2613

IIRC, A*A->A if A is a NaN, so fabs(x*x) isn't equivalent to x*x.

arsenm added inline comments.Jan 4 2017, 8:48 AM
lib/Analysis/ValueTracking.cpp
2612–2613

I think the sign of a NaN is OK to ignore

scanon added inline comments.Jan 4 2017, 9:28 AM
lib/Analysis/ValueTracking.cpp
2612–2613

It's OK [in the IEEE 754 sense] to ignore the sign of NaN with all operations *except* abs, copysign, copy, and negate. So fabs(x) is not equivalent to x if x is NaN, but fabs(x*x) -> x*x is OK if x might be NaN, because the sign of x*x has no meaning if x is NaN.

efriedma added inline comments.Jan 4 2017, 11:18 AM
lib/Analysis/ValueTracking.cpp
2612–2613

Okay, since there seems to be some confusion here, I went and took a look at the standard. It says "For all other operations, this standard does not specify the sign bit of a NaN result." So x*x has an unspecified sign, and fabs(x*x) has a sign bit which is always clear.

From this we can conclude that something like fabs(x*x) + 1 -> x*x + 1 is a legal transformation, but the general transform proposed here is not legal. For example, copysign(1, fabs(x*x)) == 1, but copysign(1, x*x) could be either 1 or -1 depending on the target.

scanon added inline comments.Jan 4 2017, 11:28 AM
lib/Analysis/ValueTracking.cpp
2612–2613

Correct. Sorry for the confusion.

arsenm updated this revision to Diff 83656.Jan 9 2017, 11:07 AM

Handle x * x case correctly

efriedma accepted this revision.Jan 9 2017, 11:29 AM
efriedma edited edge metadata.

LGTM.

include/llvm/Analysis/ValueTracking.h
179

Minor tweak: Depth is never explicitly passed in, so might as well get rid of the argument.

This revision is now accepted and ready to land.Jan 9 2017, 11:29 AM
arsenm closed this revision.Jan 10 2017, 4:45 PM

r291624 with fmul nan fix logic also applied to the FMA case