[Sema] Diagnose floating point conversions based on target semantics

[erik.pilkington]

Summary

...instead of just comparing rank. Also, fix a bad warning about _Float16, since its declared out of order in BuiltinTypes.def, meaning comparing rank using BuiltinType::getKind() is incorrect.

Came out of some discussion on D58145

Thanks!

[rjmccall]

LGTM, thanks!

include/clang/AST/ASTContext.h

public:
 
   /// Compare the rank of the two specified floating point types,
   /// ignoring the domain of the type (i.e. 'double' == '_Complex double').
   ///
   /// If \p LHS > \p RHS, returns 1.  If \p LHS == \p RHS, returns 0.  If
   /// \p LHS < \p RHS, return -1.
   int getFloatingTypeOrder(QualType LHS, QualType RHS) const;
 
+  /// Compare the rank of two floating point types as above, but compare equal
+  /// if both types have the same floating-point semantics on the target (i.e.
+  /// long double and double on AArch64 will return 0).
+  int getFloatingTypeSemanticOrder(QualType LHS, QualType RHS) const;
+
   /// Return a real floating point or a complex type (based on
   /// \p typeDomain/\p typeSize).
   ///
   /// \param typeDomain a real floating point or complex type.
   /// \param typeSize a real floating point or complex type.
   QualType getFloatingTypeOfSizeWithinDomain(QualType typeSize,
                                              QualType typeDomain) const;
 

lib/AST/ASTContext.cpp

int ASTContext::getFloatingTypeOrder(QualType LHS, QualType RHS) const {
 
   if (LHSR == RHSR)
     return 0;
   if (LHSR > RHSR)
     return 1;
   return -1;
 }
 
+int ASTContext::getFloatingTypeSemanticOrder(QualType LHS, QualType RHS) const {
+  if (&getFloatTypeSemantics(LHS) == &getFloatTypeSemantics(RHS))
+    return 0;
+  return getFloatingTypeOrder(LHS, RHS);
+}
+
 /// getIntegerRank - Return an integer conversion rank (C99 6.3.1.1p1). This
 /// routine will assert if passed a built-in type that isn't an integer or enum,
 /// or if it is not canonicalized.
 unsigned ASTContext::getIntegerRank(const Type *T) const {
   assert(T->isCanonicalUnqualified() && "T should be canonicalized");
 
   switch (cast<BuiltinType>(T)->getKind()) {
   default: llvm_unreachable("getIntegerRank(): not a built-in integer");

lib/Sema/SemaChecking.cpp

const auto *RBT = cast<CompoundAssignOperator>(E)
                         ->getComputationResultType()
                         ->getAs<BuiltinType>();
 
   // The below checks assume source is floating point.
   if (!ResultBT || !RBT || !RBT->isFloatingPoint()) return;
 
   // If source is floating point but target is an integer.
   if (ResultBT->isInteger())
-    DiagnoseImpCast(S, E, E->getRHS()->getType(), E->getLHS()->getType(),
+    return DiagnoseImpCast(S, E, E->getRHS()->getType(), E->getLHS()->getType(),
                            E->getExprLoc(), diag::warn_impcast_float_integer);
-  // If both source and target are floating points. Builtin FP kinds are ordered
-  // by increasing FP rank. FIXME: except _Float16, we currently emit a bogus
-  // warning.
-  else if (ResultBT->isFloatingPoint() && ResultBT->getKind() < RBT->getKind() &&
-           // We don't want to warn for system macro.
-           !S.SourceMgr.isInSystemMacro(E->getOperatorLoc()))
+
+  if (!ResultBT->isFloatingPoint())
+    return;
+
+  // If both source and target are floating points, warn about losing precision.
+  int Order = S.getASTContext().getFloatingTypeSemanticOrder(
+      QualType(ResultBT, 0), QualType(RBT, 0));
+  if (Order < 0 && !S.SourceMgr.isInSystemMacro(E->getOperatorLoc()))
     // warn about dropping FP rank.
     DiagnoseImpCast(S, E->getRHS(), E->getLHS()->getType(), E->getOperatorLoc(),
                     diag::warn_impcast_float_result_precision);
 }
 
 static std::string PrettyPrintInRange(const llvm::APSInt &Value,
                                       IntRange Range) {
   if (!Range.Width) return "0";
CheckImplicitConversion(Sema &S, Expr *E, QualType T, SourceLocation CC,
   const BuiltinType *TargetBT = dyn_cast<BuiltinType>(Target);
 
   // If the source is floating point...
   if (SourceBT && SourceBT->isFloatingPoint()) {
     // ...and the target is floating point...
     if (TargetBT && TargetBT->isFloatingPoint()) {
       // ...then warn if we're dropping FP rank.
 
-      // Builtin FP kinds are ordered by increasing FP rank.
-      if (SourceBT->getKind() > TargetBT->getKind()) {
+      int Order = S.getASTContext().getFloatingTypeSemanticOrder(
+          QualType(SourceBT, 0), QualType(TargetBT, 0));
+      if (Order > 0) {
         // Don't warn about float constants that are precisely
         // representable in the target type.
         Expr::EvalResult result;
         if (E->EvaluateAsRValue(result, S.Context)) {
           // Value might be a float, a float vector, or a float complex.
           if (IsSameFloatAfterCast(result.Val,
                    S.Context.getFloatTypeSemantics(QualType(TargetBT, 0)),
                    S.Context.getFloatTypeSemantics(QualType(SourceBT, 0))))
             return;
         }
 
         if (S.SourceMgr.isInSystemMacro(CC))
           return;
 
         DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_float_precision);
       }
       // ... or possibly if we're increasing rank, too
-      else if (TargetBT->getKind() > SourceBT->getKind()) {
+      else if (Order < 0) {
         if (S.SourceMgr.isInSystemMacro(CC))
           return;
 
         DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_double_promotion);
       }
       return;
     }
 

test/Sema/conversion-target-dep.c

+// RUN: %clang_cc1 -Wdouble-promotion -Wimplicit-float-conversion %s -triple x86_64-apple-macosx10.12 -verify=x86,expected
+// RUN: %clang_cc1 -Wdouble-promotion -Wimplicit-float-conversion %s -triple armv7-apple-ios9.0 -verify=arm,expected
+
+// On ARM, long double and double both map to double precision 754s, so there
+// isn't any reason to warn on conversions back and forth.
+
+long double ld;
+double d;
+_Float16 f16; // x86-error {{_Float16 is not supported on this target}}
+
+int main() {
+  ld = d; // x86-warning {{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+  d = ld; // x86-warning {{implicit conversion loses floating-point precision: 'long double' to 'double'}}
+
+  ld += d; // x86-warning {{implicit conversion increases floating-point precision: 'double' to 'long double'}}
+  d += ld; // x86-warning {{implicit conversion when assigning computation result loses floating-point precision: 'long double' to 'double'}}
+
+  f16 = ld; // expected-warning {{implicit conversion loses floating-point precision: 'long double' to '_Float16'}}
+  ld = f16; // expected-warning {{implicit conversion increases floating-point precision: '_Float16' to 'long double'}}
+
+  f16 += ld; // expected-warning {{implicit conversion when assigning computation result loses floating-point precision: 'long double' to '_Float16'}}
+  ld += f16; // expected-warning {{implicit conversion increases floating-point precision: '_Float16' to 'long double'}}
+}
+