[PATCH] D24011: [ConstantFold] Add a flag for ppc_fp128 constant folding, since APFloat doesn't support double-double semantic

[Hal Finkel via llvm-commits]

hfinkel added a comment.

Here's another possible short-term approach: Actually make the APFloat representation used for PPC long double large enough to represent ((1.0 + epsilon) - 1.0), where epsilon == 4.94066e-324. I'm under the impression that this is property that is failing for you, and it is indicative of the fact that the current 128-bit representation used cannot represent all of the numbers that the runtime long double type can represent.

As a quick experiment, I'm assuming that we don't want constant folding to change the output of this program:

  $ cat ~/eps.cpp 
  #include <iostream>
  #include <limits>
  using namespace std;
  
  int main() {
    long double x = 1.0;
    cout << (x + numeric_limits<long double>::epsilon()) - x << "\n";
  }

which can be accomplished by this patch:

  diff --git a/lib/Support/APFloat.cpp b/lib/Support/APFloat.cpp
  index f9370b8..81cef0e 100644
  --- a/lib/Support/APFloat.cpp
  +++ b/lib/Support/APFloat.cpp
  @@ -76,7 +76,7 @@ namespace llvm {
        to represent all possible values held by a PPC double-double number,
        for example: (long double) 1.0 + (long double) 0x1p-106
        Should this be replaced by a full emulation of PPC double-double?  */
  -  const fltSemantics APFloat::PPCDoubleDouble = { 1023, -1022 + 53, 53 + 53, 128 };
  +  const fltSemantics APFloat::PPCDoubleDouble = { 1023, -1022 + 53, 20*(53 + 53), 128 };
   
     /* A tight upper bound on number of parts required to hold the value
        pow(5, power) is
  @@ -2926,7 +2926,7 @@ APInt
   APFloat::convertPPCDoubleDoubleAPFloatToAPInt() const
   {
     assert(semantics == (const llvm::fltSemantics*)&PPCDoubleDouble);
  -  assert(partCount()==2);
  +  // assert(partCount()==2);
   
     uint64_t words[2];
     opStatus fs;
  @@ -2947,7 +2947,7 @@ APFloat::convertPPCDoubleDoubleAPFloatToAPInt() const
   
     APFloat u(extended);
     fs = u.convert(IEEEdouble, rmNearestTiesToEven, &losesInfo);
  -  assert(fs == opOK || fs == opInexact);
  +  // assert(fs == opOK || fs == opInexact);
     (void)fs;
     words[0] = *u.convertDoubleAPFloatToAPInt().getRawData();
   
  @@ -2963,7 +2963,7 @@ APFloat::convertPPCDoubleDoubleAPFloatToAPInt() const
       APFloat v(extended);
       v.subtract(u, rmNearestTiesToEven);
       fs = v.convert(IEEEdouble, rmNearestTiesToEven, &losesInfo);
  -    assert(fs == opOK && !losesInfo);
  +    // assert(fs == opOK && !losesInfo);
       (void)fs;
       words[1] = *v.convertDoubleAPFloatToAPInt().getRawData();
     } else {

and with this patch applied, even when compiled with optimizations enabled, the test program still prints "4.94066e-324" like it should. This is obviously also a work-around, but:

1. It should only cause APFloat answers to differ from runtime answers by being more accurate, not less.
2. It isolates the work-around to the APFloat implementation, instead of trying to spread the knowledge of the APFloat deficiencies around the rest of the infrastructure.

P.S. With this patch applied, all Clang regression tests pass, and all LLVM regression tests pass except for the ADT/ADTTests/APFloatTest.PPCDoubleDouble unit test (for a few checks that are checking the representation itself).


https://reviews.llvm.org/D24011