Page MenuHomePhabricator

Fix APFloat mod
ClosedPublic

Authored by simonbyrne on Jan 31 2017, 1:19 PM.

Details

Summary

The existing code for APFloat mod was incorrect in two ways:

  • intermediate rounding gave incorrect results in some cases (e.g. frem double 0.3 0.01)
  • the method of integer truncation could cause intermediate overflow

This caused subsequent errors in the constant propagation pass, see https://llvm.org/bugs/show_bug.cgi?id=3316

Diff Detail

Repository
rL LLVM

Event Timeline

simonbyrne created this revision.Jan 31 2017, 1:19 PM
simonbyrne updated this revision to Diff 86589.Feb 1 2017, 1:39 AM

Use fusedMultiplyAdd.

scanon edited edge metadata.Feb 1 2017, 5:58 AM

Numerically this is sound, and something that I've wanted to fix for a while but haven't had time to deal with. Thanks for doing it! Please add some test cases that would detect the old error (@gottesmm can likely point you in the right direction).

tkelman added a subscriber: tkelman.Feb 1 2017, 7:09 AM

Thanks. I'm not really that familiar with the layout of the repository: where should tests go?

Thanks. I'm not really that familiar with the layout of the repository: where should tests go?

/llvm/unittests/ADT/APFloatTest.cpp

simonbyrne updated this revision to Diff 87374.Feb 7 2017, 1:43 AM

Changed to use subtraction to avoid intermediate overflow, added tests.

scanon added a comment.Feb 7 2017, 9:49 AM

Great, thanks Simon! This looks fine numerically. @gottesmm, can you double-check for style, etc?

gottesmm requested changes to this revision.Feb 8 2017, 1:12 PM

Did you run this through git-clang-format?

This revision now requires changes to proceed.Feb 8 2017, 1:12 PM
simonbyrne updated this revision to Diff 87707.Feb 8 2017, 1:40 PM
simonbyrne edited edge metadata.

Updated to use git clang-format

gottesmm accepted this revision.Feb 8 2017, 2:57 PM

Thanks for running it through git-clang-format. LGTM!

This revision is now accepted and ready to land.Feb 8 2017, 2:57 PM
sanjoy added a subscriber: sanjoy.Mar 8 2017, 12:42 PM
This revision was automatically updated to reflect the committed changes.

This patch causes incorrect functional behavior, specifically, IEEE specification requires the sign of the remainder is the same as numerator in case remainder is zero.
It is incorrect with this patch. The following tests can be used to show the issue:

{
  APFloat f1(APFloat::IEEEdouble(), "-4.0");
  APFloat f2(APFloat::IEEEdouble(), "-2.0");
  APFloat expected(APFloat::IEEEdouble(), "-0.0");
  EXPECT_EQ(f1.mod(f2), APFloat::opOK);
  EXPECT_TRUE(f1.bitwiseIsEqual(expected));
}
{
  APFloat f1(APFloat::IEEEdouble(), "-4.0");
  APFloat f2(APFloat::IEEEdouble(), "2.0");
  APFloat expected(APFloat::IEEEdouble(), "-0.0");
  EXPECT_EQ(f1.mod(f2), APFloat::opOK);
  EXPECT_TRUE(f1.bitwiseIsEqual(expected));
}

I plan to revert this patch and add mentioned tests...

Actually, I've upload a fix, please review it.