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

[Hubert Tong via cfe-commits]

hubert.reinterpretcast added inline comments.

================
Comment at: lib/CodeGen/CGDebugInfo.cpp:550
@@ -548,1 +549,3 @@
   case BuiltinType::Double:
+    // FIXME: For targets where lond double and __float128 have the same size,
+    // they are currently indistinguishable in the debugger withouth some
----------------
There are various typos in this comment.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:1809
@@ +1808,3 @@
+      else if (value->getType()->isHalfTy())
+        FS = &CGF.getTarget().getHalfFormat();
+      F.convert(*FS, llvm::APFloat::rmTowardZero, &ignored);
----------------
It might make sense to move the call to `getLongDoubleFormat()` to the if/else block and perhaps to order it as long double/__float128/half.

================
Comment at: lib/Sema/SemaExpr.cpp:1115
@@ -1114,3 +1114,3 @@
     int order = S.Context.getFloatingTypeOrder(LHSType, RHSType);
-    if (order > 0) {
+    if (order >= 0) {
       RHS = S.ImpCastExprToType(RHS.get(), LHSType, CK_FloatingCast);
----------------
Are we all comfortable that if `long double` and `__float128` have the same representation, but are considered distinct types, then the LHS type is used? Is this consistent with GCC?

================
Comment at: lib/Sema/SemaExpr.cpp:1146
@@ +1145,3 @@
+                               QualType RHSType) {
+  if (!LHSType.getTypePtr()->isFloatingType() ||
+      !RHSType.getTypePtr()->isFloatingType() ||
----------------
There's no actual need for the `.getTypePtr()` part; the overloaded `operator->` does the same thing.

================
Comment at: lib/Sema/SemaExpr.cpp:1153
@@ +1152,3 @@
+  QualType LHSElemType = LHSType->isComplexType() ?
+    LHSComplexType->getElementType() : LHSType;
+  QualType RHSElemType = RHSType->isComplexType() ?
----------------
`LHSComplexType` is only used here. It could be replaced with `cast<ComplexType>(LHSType.getTypePtr())` (and similarly for the RHS).

Indeed, since we are checking against specific real element types anyway, `dyn_cast<ComplexType>(LHSType.getTypePtr())` (and checking for null) would suffice (no need to use `isComplexType()`).

================
Comment at: test/Sema/float128-ld-incompatibility.cpp:32
@@ +31,2 @@
+  q / ld; // expected-error {{invalid operands to binary expression ('__float128' and 'long double')}}
+}
----------------
I believe a test for the conditional operator would be appropriate.


Repository:
  rL LLVM

http://reviews.llvm.org/D15120