[PATCH] D15120: Add support for __float128 type to be used by targets that support it

[Nemanja Ivanovic via cfe-commits]

nemanjai added inline comments.

================
Comment at: lib/Basic/TargetInfo.cpp:231
@@ +230,3 @@
+    if (hasFloat128Type() &&
+        &getFloat128Format() == &llvm::APFloat::IEEEquad)
+      return Float128;
----------------
hubert.reinterpretcast wrote:
> Is it necessary to check that `__float128` is IEEE quad here? Unlike the long double cases, `__float128` really better be 128 bits.
It would indeed be weird if it wasn't so. I can remove the second condition in that if.

================
Comment at: lib/Sema/SemaExpr.cpp:1169
@@ +1168,3 @@
+  if (LHSElemType == S.Context.Float128Ty &&
+      RHSElemType == S.Context.LongDoubleTy)
+    return true;
----------------
hubert.reinterpretcast wrote:
> I do not believe that the width of `long double` is sufficiently established to be the same as that of `__float128` in this context.
This would certainly cause any attempts to convert between some different implementation of **`long double` **(i.e. the Intel 80-bit type). I believe that this behaviour is desired in all cases where **`long double`** and **`double`** have a different representation. Namely, we do not currently have any support for converting between **`fp128`** and anything that has precision >= **`double`** that is not actually **`double`**.

What I propose to do here and in other locations where we diagnose conversions between the two types is that the check is:
- One type is `__float128`
- The other is `long double`
- The representation of `long double` is not `llvm::APFloat::IEEEdouble`

Basically in this function, the early exit path would be:
- representations are the same or
- representation of `long double` is `llvm::APFloat::IEEEdouble`

================
Comment at: lib/Sema/SemaExpr.cpp:1341
@@ +1340,3 @@
+  // Diagnose builtin types that have the same size and different representation
+  // as conversions between such type currently can't be handled.
+  if (sameWidthDifferentRepresentation(*this, LHSType, RHSType))
----------------
hubert.reinterpretcast wrote:
> s/such type/such types;
Oops, I dropped the s. I'll fix it.

================
Comment at: lib/Sema/SemaOverload.cpp:1655
@@ -1654,1 +1654,3 @@
   } else if (FromType->isRealFloatingType() && ToType->isRealFloatingType()) {
+    // FIXME: disable conversions between long double and __float128 if
+    // their representation is different until there is back end support
----------------
hubert.reinterpretcast wrote:
> Is conversion between `long double` and `__float128` the correct characterization of the missing back-end support? (as opposed to conversion between `PPCDoubleDouble` and `IEEEquad`)
Well, the same issue exists with `x86_fp80`. We don't have libcalls set up for handling that either. Of course the intended semantics and the comment are still not quite correct here. Really, what I think we should be after is catching attempts to convert between `__float128` and `long double` on targets where `long double` is not actually just `double`.


Repository:
  rL LLVM

http://reviews.llvm.org/D15120