[APFloat] Prevent construction of APFloat with Semantics and FP value

[ekatz]

Summary

Constructor invocations such as APFloat(APFloat::IEEEdouble(), 0.0) may seem like they accept a FP (floating point) value, but the overload they reach is actually the integerPart one, not a float or double overload (which only exists when fltSemantics isn't passed).

This may lead to possible loss of data, by the conversion from float or double to 'integerPart'.

To prevent future mistakes, a new constructor overload, which accepts any FP value and marked with delete, to prevent its usage.

Fix PR34095.

[grandinj]

Perhaps there should be an "= delete" constructor to make this a compile error?

[hfinkel]

In D70425#1751548, @grandinj wrote:

Perhaps there should be an "= delete" constructor to make this a compile error?

I agree. This seems, as evidenced by our current code, confusing. Make we should have a private constructor for this and clients need to use some APFloat::createFromInt or similar to get this behavior?

[ekatz]

Added a new constructor overload, which accepts any FP value and marked with delete, to prevent its usage.

This must be done with SFINAE, otherwise (a simple overload for double/float) we get "ambiguous call to overloaded function", unless we overload each specific type we allow (not only integerPart).

[ekatz]

Ping

[efriedma]

LGTM

llvm/include/llvm/ADT/APFloat.h

if (usesLayout<DoubleAPFloat>(getSemantics()))
       return U.Double.compareAbsoluteValue(RHS.U.Double);
     llvm_unreachable("Unexpected semantics");
   }
 
 public:
   APFloat(const fltSemantics &Semantics) : U(Semantics) {}
   APFloat(const fltSemantics &Semantics, StringRef S);
   APFloat(const fltSemantics &Semantics, integerPart I) : U(Semantics, I) {}
+  template <typename T, typename = typename std::enable_if<
+                            std::is_floating_point<T>::value>::type>
+  APFloat(const fltSemantics &Semantics, T V) = delete;
   // TODO: Remove this constructor. This isn't faster than the first one.
   APFloat(const fltSemantics &Semantics, uninitializedTag)
       : U(Semantics, uninitialized) {}
   APFloat(const fltSemantics &Semantics, const APInt &I) : U(Semantics, I) {}
   explicit APFloat(double d) : U(IEEEFloat(d), IEEEdouble()) {}
   explicit APFloat(float f) : U(IEEEFloat(f), IEEEsingle()) {}
   APFloat(const APFloat &RHS) = default;
   APFloat(APFloat &&RHS) = default;

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

case Intrinsic::amdgcn_rcp: {
     Value *Src = II->getArgOperand(0);
 
     // TODO: Move to ConstantFolding/InstSimplify?
     if (isa<UndefValue>(Src))
       return replaceInstUsesWith(CI, Src);
 
     if (const ConstantFP *C = dyn_cast<ConstantFP>(Src)) {
       const APFloat &ArgVal = C->getValueAPF();
-      APFloat Val(ArgVal.getSemantics(), 1.0);
+      APFloat Val(ArgVal.getSemantics(), 1);
       APFloat::opStatus Status = Val.divide(ArgVal,
                                             APFloat::rmNearestTiesToEven);
       // Only do this if it was exact and therefore not dependent on the
       // rounding mode.
       if (Status == APFloat::opOK)
         return replaceInstUsesWith(CI, ConstantFP::get(II->getContext(), Val));
     }
 

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

Value *LibCallSimplifier::optimizePow(CallInst *Pow, IRBuilder<> &B) {
   const APFloat *ExpoF;
   if (AllowApprox && match(Expo, m_APFloat(ExpoF))) {
     // We limit to a max of 7 multiplications, thus the maximum exponent is 32.
     // If the exponent is an integer+0.5 we generate a call to sqrt and an
     // additional fmul.
     // TODO: This whole transformation should be backend specific (e.g. some
     //       backends might prefer libcalls or the limit for the exponent might
     //       be different) and it should also consider optimizing for size.
-    APFloat LimF(ExpoF->getSemantics(), 33.0),
+    APFloat LimF(ExpoF->getSemantics(), 33),
             ExpoA(abs(*ExpoF));
     if (ExpoA.compare(LimF) == APFloat::cmpLessThan) {
       // This transformation applies to integer or integer+0.5 exponents only.
       // For integer+0.5, we create a sqrt(Base) call.
       Value *Sqrt = nullptr;
       if (!ExpoA.isInteger()) {
         APFloat Expo2 = ExpoA;
         // To check if ExpoA is an integer + 0.5, we add it to itself. If there

llvm/unittests/ADT/APFloatTest.cpp

// Test -ve sign preservation when small negative results underflow.
     APFloat f2(APFloat::IEEEdouble(), "+0x1p-1074");
     APFloat f3(0.0);
     f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
     EXPECT_TRUE(f1.isNegative() && f1.isZero());
   }
 
   // Test x87 extended precision case from http://llvm.org/PR20728.
   {
-    APFloat M1(APFloat::x87DoubleExtended(), 1.0);
-    APFloat M2(APFloat::x87DoubleExtended(), 1.0);
-    APFloat A(APFloat::x87DoubleExtended(), 3.0);
+    APFloat M1(APFloat::x87DoubleExtended(), 1);
+    APFloat M2(APFloat::x87DoubleExtended(), 1);
+    APFloat A(APFloat::x87DoubleExtended(), 3);
 
     bool losesInfo = false;
     M1.fusedMultiplyAdd(M1, A, APFloat::rmNearestTiesToEven);
     M1.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
     EXPECT_FALSE(losesInfo);
     EXPECT_EQ(4.0f, M1.convertToFloat());
   }
 

 
 TEST(APFloatTest, Denormal) {
   APFloat::roundingMode rdmd = APFloat::rmNearestTiesToEven;
 
   // Test single precision
   {
     const char *MinNormalStr = "1.17549435082228750797e-38";
     EXPECT_FALSE(APFloat(APFloat::IEEEsingle(), MinNormalStr).isDenormal());
-    EXPECT_FALSE(APFloat(APFloat::IEEEsingle(), 0.0).isDenormal());
+    EXPECT_FALSE(APFloat(APFloat::IEEEsingle(), 0).isDenormal());
 
-    APFloat Val2(APFloat::IEEEsingle(), 2.0e0);
+    APFloat Val2(APFloat::IEEEsingle(), 2);
     APFloat T(APFloat::IEEEsingle(), MinNormalStr);
     T.divide(Val2, rdmd);
     EXPECT_TRUE(T.isDenormal());
   }
 
   // Test double precision
   {
     const char *MinNormalStr = "2.22507385850720138309e-308";
     EXPECT_FALSE(APFloat(APFloat::IEEEdouble(), MinNormalStr).isDenormal());
-    EXPECT_FALSE(APFloat(APFloat::IEEEdouble(), 0.0).isDenormal());
+    EXPECT_FALSE(APFloat(APFloat::IEEEdouble(), 0).isDenormal());
 
-    APFloat Val2(APFloat::IEEEdouble(), 2.0e0);
+    APFloat Val2(APFloat::IEEEdouble(), 2);
     APFloat T(APFloat::IEEEdouble(), MinNormalStr);
     T.divide(Val2, rdmd);
     EXPECT_TRUE(T.isDenormal());
   }
 
   // Test Intel double-ext
   {
     const char *MinNormalStr = "3.36210314311209350626e-4932";
     EXPECT_FALSE(APFloat(APFloat::x87DoubleExtended(), MinNormalStr).isDenormal());
-    EXPECT_FALSE(APFloat(APFloat::x87DoubleExtended(), 0.0).isDenormal());
+    EXPECT_FALSE(APFloat(APFloat::x87DoubleExtended(), 0).isDenormal());
 
-    APFloat Val2(APFloat::x87DoubleExtended(), 2.0e0);
+    APFloat Val2(APFloat::x87DoubleExtended(), 2);
     APFloat T(APFloat::x87DoubleExtended(), MinNormalStr);
     T.divide(Val2, rdmd);
     EXPECT_TRUE(T.isDenormal());
   }
 
   // Test quadruple precision
   {
     const char *MinNormalStr = "3.36210314311209350626267781732175260e-4932";
     EXPECT_FALSE(APFloat(APFloat::IEEEquad(), MinNormalStr).isDenormal());
-    EXPECT_FALSE(APFloat(APFloat::IEEEquad(), 0.0).isDenormal());
+    EXPECT_FALSE(APFloat(APFloat::IEEEquad(), 0).isDenormal());
 
-    APFloat Val2(APFloat::IEEEquad(), 2.0e0);
+    APFloat Val2(APFloat::IEEEquad(), 2);
     APFloat T(APFloat::IEEEquad(), MinNormalStr);
     T.divide(Val2, rdmd);
     EXPECT_TRUE(T.isDenormal());
   }
 }
 
 TEST(APFloatTest, Zero) {
   EXPECT_EQ(0.0f,  APFloat(0.0f).convertToFloat());
TEST(APFloatTest, makeNaN) {
   for (const auto &t : tests) {
     ASSERT_EQ(t.expected, nanbitsFromAPInt(t.semantics, t.SNaN, t.Negative, t.payload));
   }
 }
 
 #ifdef GTEST_HAS_DEATH_TEST
 #ifndef NDEBUG
 TEST(APFloatTest, SemanticsDeath) {
-  EXPECT_DEATH(APFloat(APFloat::IEEEsingle(), 0.0f).convertToDouble(), "Float semantics are not IEEEdouble");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), 0.0 ).convertToFloat(),  "Float semantics are not IEEEsingle");
+  EXPECT_DEATH(APFloat(APFloat::IEEEsingle(), 0).convertToDouble(), "Float semantics are not IEEEdouble");
+  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), 0).convertToFloat(),  "Float semantics are not IEEEsingle");
 }
 
 TEST(APFloatTest, StringDecimalDeath) {
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  ""), "Invalid string length");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+"), "String has no digits");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "-"), "String has no digits");
 
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("\0", 1)), "Invalid character in significand");