[lldb-dev] [llvm] r189624 - Change default # of digits for APFloat::toString

[Thirumurthi, Ashok]

Hi Eli,

I noticed that your commit introduces a regression in the LLDB test suite because expression evaluation of a floating point constant like 2.234f results in a value like 2.23399997.  I suspect that this occurs because 2.234f is really just the closest number to 2.23399997 that can be represented using floating point precision.  I noticed that your commit increases the default number of digits in the precision of APFloat.  I can see how that's useful when performing intermediate computation, but I would have expected APFloat::toString to cleverly avoid reality.

The attached black magic fixes the regressions in the IRInterpreter used for expression evaluation.  However, I'm wondering if the correct fix is to revert your commit (i.e. in favor of mode to select the default precision as nominal precision versus active precision).  Cheers,

- Ashok


-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Eli Friedman
Sent: Thursday, August 29, 2013 7:45 PM
To: llvm-commits at cs.uiuc.edu
Subject: [llvm] r189624 - Change default # of digits for APFloat::toString

Author: efriedma
Date: Thu Aug 29 18:44:34 2013
New Revision: 189624

URL: http://llvm.org/viewvc/llvm-project?rev=189624&view=rev
Log:
Change default # of digits for APFloat::toString

This is a re-commit of r189442; I'll follow up with clang changes.

The previous default was almost, but not quite enough digits to represent a floating-point value in a manner which preserves the representation when it's read back in.  The larger default is much less confusing.

I spent some time looking into printing exactly the right number of digits if a precision isn't specified, but it's kind of complicated, and I'm not really sure I understand what APFloat::toString is supposed to output for FormatPrecision != 0 (or maybe the current API specification is just silly, not sure which).  I have a WIP patch if anyone is interested.

Modified:
    llvm/trunk/lib/Support/APFloat.cpp
    llvm/trunk/unittests/ADT/APFloatTest.cpp

Modified: llvm/trunk/lib/Support/APFloat.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/APFloat.cpp?rev=189624&r1=189623&r2=189624&view=diff
==============================================================================
--- llvm/trunk/lib/Support/APFloat.cpp (original)
+++ llvm/trunk/lib/Support/APFloat.cpp Thu Aug 29 18:44:34 2013
@@ -3546,11 +3546,14 @@ void APFloat::toString(SmallVectorImpl<c
   // Set FormatPrecision if zero.  We want to do this before we
   // truncate trailing zeros, as those are part of the precision.
   if (!FormatPrecision) {
-    // It's an interesting question whether to use the nominal
-    // precision or the active precision here for denormals.
+    // We use enough digits so the number can be round-tripped back to an
+    // APFloat. The formula comes from "How to Print Floating-Point Numbers
+    // Accurately" by Steele and White.
+    // FIXME: Using a formula based purely on the precision is conservative;
+    // we can print fewer digits depending on the actual value being printed.
 
-    // FormatPrecision = ceil(significandBits / lg_2(10))
-    FormatPrecision = (semantics->precision * 59 + 195) / 196;
+    // FormatPrecision = 2 + floor(significandBits / lg_2(10))
+    FormatPrecision = 2 + semantics->precision * 59 / 196;
   }
 
   // Ignore trailing binary zeros.

Modified: llvm/trunk/unittests/ADT/APFloatTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/APFloatTest.cpp?rev=189624&r1=189623&r2=189624&view=diff
==============================================================================
--- llvm/trunk/unittests/ADT/APFloatTest.cpp (original)
+++ llvm/trunk/unittests/ADT/APFloatTest.cpp Thu Aug 29 18:44:34 2013
@@ -866,10 +866,11 @@ TEST(APFloatTest, toString) {
   ASSERT_EQ("0.0101", convertToString(1.01E-2, 5, 2));
   ASSERT_EQ("0.0101", convertToString(1.01E-2, 4, 2));
   ASSERT_EQ("1.01E-2", convertToString(1.01E-2, 5, 1));
-  ASSERT_EQ("0.7853981633974483", convertToString(0.78539816339744830961, 0, 3));
-  ASSERT_EQ("4.940656458412465E-324", convertToString(4.9406564584124654e-324, 0, 3));
-  ASSERT_EQ("873.1834", convertToString(873.1834, 0, 1));
-  ASSERT_EQ("8.731834E+2", convertToString(873.1834, 0, 0));
+  ASSERT_EQ("0.78539816339744828", 
+ convertToString(0.78539816339744830961, 0, 3));  
+ ASSERT_EQ("4.9406564584124654E-324", 
+ convertToString(4.9406564584124654e-324, 0, 3));  
+ ASSERT_EQ("873.18340000000001", convertToString(873.1834, 0, 1));  
+ ASSERT_EQ("8.7318340000000001E+2", convertToString(873.1834, 0, 0));  
+ ASSERT_EQ("1.7976931348623157E+308", 
+ convertToString(1.7976931348623157E+308, 0, 0));
 }
 
 TEST(APFloatTest, toInteger) {


_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: TestExpr.patch
Type: application/octet-stream
Size: 1012 bytes
Desc: TestExpr.patch
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20130904/1ce0a4ed/attachment.obj>