Page MenuHomePhabricator

InstCombine: fabs(x) * fabs(x) -> x * x
ClosedPublic

Authored by arsenm on Jan 28 2016, 3:22 PM.

Details

Reviewers
scanon

Diff Detail

Event Timeline

arsenm updated this revision to Diff 46319.Jan 28 2016, 3:22 PM
arsenm retitled this revision from to InstCombine: fabs(x) * fabs(x) -> x * x.
arsenm updated this object.
arsenm added a subscriber: llvm-commits.
arsenm updated this revision to Diff 46321.Jan 28 2016, 3:35 PM

Make sure name is kept

mgrang added a subscriber: mgrang.Jan 28 2016, 3:41 PM
mgrang added inline comments.
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
621

Should this transformation also be guarded under AllowReassociate?

622

Would it be better to move the checks for the IntrinsicID under a switch-case?

switch (II->getIntrinsicID())

arsenm added inline comments.Jan 28 2016, 3:44 PM
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
621

I'm pretty sure this is always safe

622

Maybe, but since there are only two and sqrt has an additional condition, the switch would probably be longer

DavidKreitzer added inline comments.Jan 29 2016, 1:36 PM
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
621

If X is -NaN, this transform will change the result of fabs(X) * fabs(X) from +NaN to -NaN. (In all other cases, it will produce the same result.)

So my recommendation is to guard this transformation by I.hasNoNaNs().

deadalnix added inline comments.
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
621

My understanding of the standard is that NaN has a sign in order to avoid special casing it for operation like fabs. There is no expectation that it has a well defined value.

spatel added a subscriber: spatel.Jan 29 2016, 1:59 PM

I forgot that we had intrinsic/lib folds outside of SimplifyLibCalls. Sorry, if I've missed it, but Is there a reason it's better to do this (and the sqrt and log opts) here?

cc'ing Steve about the NaN behavior.

I forgot that we had intrinsic/lib folds outside of SimplifyLibCalls. Sorry, if I've missed it, but Is there a reason it's better to do this (and the sqrt and log opts) here?

I'm not a huge fan of SimplifyLibCalls and would prefer optimizations stick to the intrinsics. SimplifyLibCalls has the assumption that you you only can optimize when TargetLibraryInfo reports having a sqrt lib call (even for the intrinsic). This is not helpful for targets that do not have libcalls, and may have hardware instructions or custom expansions for the operations. Especially since we treat the math intrinsics as "fast" versions with looser rules than the library function, I don't think we should conflate the intrinsics and library calls so much.

I would prefer if optimizations stuck to the intrinsic whenever possible. See my RFC from a couple days ago: http://lists.llvm.org/pipermail/llvm-dev/2016-January/094593.html

scanon accepted this revision.Jan 29 2016, 2:31 PM
scanon edited edge metadata.

The signbit of NaN explicitly has no meaning, so there's no concern there. LGTM.

This revision is now accepted and ready to land.Jan 29 2016, 2:31 PM
arsenm closed this revision.Jan 29 2016, 9:09 PM

r259295