Page MenuHomePhabricator

[InstCombine] transform masking off of an FP sign bit into a fabs() intrinsic call (PR24886)
ClosedPublic

Authored by spatel on Sep 22 2015, 3:43 PM.

Details

Summary

This is a partial fix for PR24886:
https://llvm.org/bugs/show_bug.cgi?id=24886

Without this IR transform, the backend (x86 at least) was producing inefficient code.

This patch is making 2 assumptions:

  1. The canonical form of a fabs() operation is, in fact, the LLVM fabs() intrinsic.
  2. The high bit of an FP value is always the sign bit; as noted in the bug report, this isn't specified by the LangRef.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 35429.Sep 22 2015, 3:43 PM
spatel retitled this revision from to [InstCombine] transform masking off of an FP sign bit into a fabs() intrinsic call (PR24886).
spatel updated this object.
spatel added reviewers: hfinkel, ab, sanjoy.
spatel added a subscriber: llvm-commits.

If we will canonicalize away from bitmath, can we please teach computeKnownBitsFromOperator how to reason about llvm.fabs? Otherwise, this might be a pessimization.

If we will canonicalize away from bitmath, can we please teach computeKnownBitsFromOperator how to reason about llvm.fabs? Otherwise, this might be a pessimization.

Sure, that seems fair. I see we already have that kind of logic for a few other intrinsics, so fabs should slide right in there.

sanjoy added inline comments.Sep 23 2015, 10:03 PM
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1471 ↗(On Diff #35429)

I think this comment should now be sunk to the inner if.

1507 ↗(On Diff #35429)

Is this optimization valid if the cast is actually an fptosi or fptoui? They're also CastInsts.

spatel marked 2 inline comments as done.Sep 24 2015, 1:37 PM
spatel added inline comments.
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1471 ↗(On Diff #35429)

Yes - fixed.

1507 ↗(On Diff #35429)

No, it should only fire on a bitcast.
Thanks for catching that! I've added an additional check and test case to make sure we're not altering an fptoui.

spatel updated this revision to Diff 35670.Sep 24 2015, 1:39 PM
spatel marked 2 inline comments as done.

Patch updated:

  1. Moved comment that became displaced.
  2. Added check and test case to make sure that we're only transforming a bitcast (not a fptoui).

If we will canonicalize away from bitmath, can we please teach computeKnownBitsFromOperator how to reason about llvm.fabs? Otherwise, this might be a pessimization.

Sure, that seems fair. I see we already have that kind of logic for a few other intrinsics, so fabs should slide right in there.

Except that computeKnownBits and friends make assumptions about only operating on integers and scalars. There's no quick fix AFAICT.
See also: https://llvm.org/bugs/show_bug.cgi?id=24942

Ping.

The requested change to computeKnownBits (just scalars for now) is in D13222.

sanjoy edited edge metadata.Oct 1 2015, 1:05 PM

I have no more comments to make, so I'll wait for @majnemer take a look.

majnemer accepted this revision.Oct 8 2015, 9:38 AM
majnemer added a reviewer: majnemer.

LGTM

This revision is now accepted and ready to land.Oct 8 2015, 9:38 AM
This revision was automatically updated to reflect the committed changes.