Page MenuHomePhabricator

transform obscured FP sign bit ops into a fabs/fneg using TLI hook
ClosedPublic

Authored by spatel on Apr 21 2016, 2:37 PM.

Details

Summary

This is effectively a revert of:
http://reviews.llvm.org/rL249702
and a reimplementation as a DAG combine for targets that have IEEE754-compliant fabs/fneg instructions.

This is intended to resolve the objections raised on the dev list:
http://lists.llvm.org/pipermail/llvm-dev/2016-April/098154.html
and:
https://llvm.org/bugs/show_bug.cgi?id=24886#c4

In the interest of patch minimalism, I've only partly enabled AArch64. PowerPC, MIPS, x86 and others can enable later.

Diff Detail

Event Timeline

spatel updated this revision to Diff 54577.Apr 21 2016, 2:37 PM
spatel retitled this revision from to transform masking off of an FP sign bit into a fabs...but only if it's legal!.
spatel updated this object.
spatel added reviewers: escha, scanon, hfinkel, t.p.northover.
spatel added a subscriber: llvm-commits.
RKSimon added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7457

Can isConstOrConstSplat be used here to allow this combine to work on vectors as well?

mcrosier removed a subscriber: mcrosier.Apr 22 2016, 5:47 AM
spatel added inline comments.Apr 22 2016, 7:00 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7457

Yes, I thought about adding that, but my guess was that it wouldn't have much impact since vector bitcasts are usually free and hopefully removed elsewhere.

Also, the InstCombine doesn't handle vectors, so this was the 1-to-1 replacement. I can add a TODO comment for vectors though.

escha edited edge metadata.Apr 22 2016, 8:28 AM

Our target has a legal FABS (it is in fact, free, as it's a modifier). But it's implemented as FADD DST, SRC.ABS, -0.0, which may modify bits other than the top bit.

Our target has a legal FABS (it is in fact, free, as it's a modifier). But it's implemented as FADD DST, SRC.ABS, -0.0, which may modify bits other than the top bit.

Ok, I've misunderstood the definition of 'legal' then. I thought that using FADD in place of FABS would be considered "Custom" rather than "Legal". This means we need a new target hook. Any ideas about what that hook would look like?

Note also that the SDNode definitions for 'FABS' and friends is unfortunately vague:
"Perform various unary floating point operations. These are inspired by libm."

escha added a comment.Apr 22 2016, 8:50 AM

As far as I know -- "Custom" means something is not legal, but has a custom lowering. It has nothing to do with how it's implemented on the target; it just means that the target has a custom legalization hook (as opposed to Expand or such, which means it doesn't have any custom hook).

We keep FABS legal until the end (much like AMDGPU or similar) -- it's emitted either by folding a modifier onto an existing instruction, or if there's no way to do that, by emitting a "no-op" add. But this happens in Select().

hfinkel edited edge metadata.Apr 26 2016, 9:37 AM

Our target has a legal FABS (it is in fact, free, as it's a modifier). But it's implemented as FADD DST, SRC.ABS, -0.0, which may modify bits other than the top bit.

If that's observable, then we'll need a TLI hook to control this. The hook probably also need to check the type (I imagine this might not be correct for ppc_fp128, for example).

Ok, I've misunderstood the definition of 'legal' then. I thought that using FADD in place of FABS would be considered "Custom" rather than "Legal". This means we need a new target hook. Any ideas about what that hook would look like?

Yes, legal means "I know this will pattern match". The pattern, however, could do anything. You should check for legal-or-custom, however. The practice of taking legal to mean cheap and custom to mean more expensive is pretty broken in many cases.

spatel updated this revision to Diff 56185.May 4 2016, 12:23 PM
spatel retitled this revision from transform masking off of an FP sign bit into a fabs...but only if it's legal! to transform obscured FP sign bit ops into a fabs/fneg using TLI hook.
spatel updated this object.
spatel edited edge metadata.

Patch updated:

  1. Instead of checking legality of fabs/fneg, provide a TLI hook that can predicate as needed.
  2. Instead of handling fabs alone, implement a fabs/fneg helper function leaving vectors and fnabs as TODO items.
escha added a comment.May 4 2016, 12:27 PM

This is a nice solution -- instead of worrying too much about LLVM float semantic definitions, just let the target say how it implements things.

spatel updated this revision to Diff 57643.May 18 2016, 10:05 AM

Ping * 2.

Patch updated: rebased and fixed code comments.

scanon edited edge metadata.May 26 2016, 9:20 AM

Numerically, I'm fine with this. Someone else can address the approach and style, etc.

majnemer edited edge metadata.Jun 2 2016, 10:09 AM

IIRC, I asked you to add support for fabs to computeKnownBits when you added this canonicalization.

Seeing as how the canonicalization is removed, I think we should undo this aspect of computeKnownBits.

IIRC, I asked you to add support for fabs to computeKnownBits when you added this canonicalization.

Yes, that was D13222 / rL249701 .

Seeing as how the canonicalization is removed, I think we should undo this aspect of computeKnownBits.

What is the advantage of removing the analysis? It is still true that an llvm fabs() intrinsic clears the sign bit in all cases, right?

IIRC, I asked you to add support for fabs to computeKnownBits when you added this canonicalization.

Yes, that was D13222 / rL249701 .

Seeing as how the canonicalization is removed, I think we should undo this aspect of computeKnownBits.

What is the advantage of removing the analysis? It is still true that an llvm fabs() intrinsic clears the sign bit in all cases, right?

I'm not sure if what we were allowed to assume about the bitpattern of float/double from the middle end.
If it's fine, then this LGTM without touching ValueTracking.

I'm not sure if what we were allowed to assume about the bitpattern of float/double from the middle end.
If it's fine, then this LGTM without touching ValueTracking.

Ah, right - the LangRef doesn't specify anything about the FP formats. Let's be safe then. I'll update this patch to revert rL249701 .

spatel updated this revision to Diff 59440.Jun 2 2016, 12:02 PM
spatel edited edge metadata.

Patch updated:
Revert r249701 ([ValueTracking] teach computeKnownBits that a fabs() clears sign bits).
The LangRef mentions IEEE format for NaN/Inf in the IR, but I'm not sure if that allows us to deduce the position of the sign bit in IR FP formats. To be safe, we'll just remove the fabs() ValueTracking logic for now.

majnemer accepted this revision.Jun 2 2016, 12:07 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 2 2016, 12:07 PM
This revision was automatically updated to reflect the committed changes.