This is an archive of the discontinued LLVM Phabricator instance.

[APFloat] Implement PPCDoubleDouble add and subtract.
ClosedPublic

Authored by timshen on Nov 17 2016, 1:50 PM.

Details

Summary

I looked at libgcc's implementation (which is based on the paper,
Software for Doubled-Precision Floating-Point Computations", by Seppo Linnainmaa,
ACM TOMS vol 7 no 3, September 1981, pages 272-283.) and made it generic to
arbitrary IEEE floats.

Diff Detail

Repository
rL LLVM

Event Timeline

timshen updated this revision to Diff 78419.Nov 17 2016, 1:50 PM
timshen retitled this revision from to [APFloat] Implement PPCDoubleDouble add and subtract..
timshen updated this object.
timshen added reviewers: hfinkel, kbarton, echristo, iteratee.
timshen added a subscriber: llvm-commits.
hfinkel edited edge metadata.Nov 18 2016, 10:25 AM

Can you please comment on how similar/different this algorithm is from the one in compiler-rt/lib/builtins/ppc/gcc_qadd.c (and other nearby files)?

timshen updated this revision to Diff 78555.Nov 18 2016, 10:59 AM
timshen edited edge metadata.

Added a comment about the paper.

timshen added a comment.EditedNov 18 2016, 11:00 AM

Can you please comment on how similar/different this algorithm is from the one in compiler-rt/lib/builtins/ppc/gcc_qadd.c (and other nearby files)?

I didn't look at compiler-rt/lib/builtins/ppc/gcc_qadd.c. These algorithms take time to reason about, so currently the only thing I know is that they are different :P. I chose libgcc's version, because it has a paper to refer to.

Can you please comment on how similar/different this algorithm is from the one in compiler-rt/lib/builtins/ppc/gcc_qadd.c (and other nearby files)?

I didn't look at compiler-rt/lib/builtins/ppc/gcc_qadd.c. These algorithms take time to reason about, so currently the only thing I know is that they are different :P. I chose libgcc's version, because it has a paper to refer to.

Okay, can you explain your comment about overflow/underflow. Can't you do it the same way that the libgcc implementations do?

timshen updated this revision to Diff 78563.Nov 18 2016, 11:47 AM

Added more comments to the implementation, and propagate all status of the internal computations to the returned status.

Can you please comment on how similar/different this algorithm is from the one in compiler-rt/lib/builtins/ppc/gcc_qadd.c (and other nearby files)?

I didn't look at compiler-rt/lib/builtins/ppc/gcc_qadd.c. These algorithms take time to reason about, so currently the only thing I know is that they are different :P. I chose libgcc's version, because it has a paper to refer to.

Okay, can you explain your comment about overflow/underflow. Can't you do it the same way that the libgcc implementations do?

You are right. libgcc just let the underlying double arithmetic modify the fenv exception flags, so we should do the same thing.

kbarton edited edge metadata.Dec 1 2016, 10:00 AM

I looked through this, but my review is mostly superficial as I don't have the background to be able to comment on the logic here.

llvm/lib/Support/APFloat.cpp
4069 ↗(On Diff #78563)

Why is this an empty string?

4076 ↗(On Diff #78563)

Is it possible for Floats[1] to ever be negative?

hubert.reinterpretcast added inline comments.
llvm/lib/Support/APFloat.cpp
4076 ↗(On Diff #78563)

The magnitude of Floats[0] should be greater than that of Floats[1].
The negative zero case may be interesting.

timshen added inline comments.Dec 1 2016, 12:10 PM
llvm/lib/Support/APFloat.cpp
4076 ↗(On Diff #78563)

Is it possible for Floats[1] to ever be negative?

It's possible, e.g. (+1.0, -1e-100), which models mathematical 1-1e-100.

And yes, the magnitude of Floats[0] is greater than Floats[1].

APFloat has special category flags for zero, so Floats[1] is completely ignored when Floats[0].getCategory() is Zero.

timshen planned changes to this revision.Dec 1 2016, 3:33 PM

I'm adding more tests.

timshen updated this revision to Diff 80958.Dec 9 2016, 2:42 PM
timshen edited edge metadata.

Added more tests and support for bitcastToAPInt (the tests use it).

I believe that now it's mostly behaving in the same way as libgcc does, except for one bug that libgcc has (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78759), but I fixed it here.

Couple of small inline nits. Willing to go with it on the implementation details.

Adding Steve in case he wishes to comment.

-eric

llvm/lib/Support/APFloat.cpp
4167 ↗(On Diff #80958)

Since you're adding a Print you might want to add a dump as well. (And while I realize it's against how the class currently works Print->print please).

4069 ↗(On Diff #78563)

Still an empty string :)

hfinkel added inline comments.Dec 12 2016, 6:32 AM
llvm/lib/Support/APFloat.cpp
3948 ↗(On Diff #80958)

It looks here like q = (a - z), but it would be useful to put that in the comment explicitly. Also, is there a reason for computing (a - (q + z)) as -((q + z) - a)?

timshen updated this revision to Diff 81128.Dec 12 2016, 12:57 PM
timshen edited edge metadata.

Removed unnecessary llvm_unreachable(), and add comments for addImpl details.

timshen added inline comments.Dec 12 2016, 1:01 PM
llvm/lib/Support/APFloat.cpp
3948 ↗(On Diff #80958)

We compute -((q + z) - a) instead of (a-(q+z)) to avoid creating temporary APFloat variables, thus avoid copying the object. Added a comment about that.

4069 ↗(On Diff #78563)

Removed unnecessary llvm_unreachable.

timshen marked 4 inline comments as done.Dec 12 2016, 1:02 PM
hfinkel accepted this revision.Dec 12 2016, 1:39 PM
hfinkel edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 12 2016, 1:39 PM
timshen updated this object.Dec 12 2016, 2:08 PM
timshen edited edge metadata.
This revision was automatically updated to reflect the committed changes.