[OpenCL] Improve floating point literal handling

[neil.hickey]

Summary

Improve handling of floating point literals for OpenCL.

If the target doesn't support cl_khr_fp64 extension, floating point literals should remain single precision.

[Anastasia]

Could we add a CodeGen test as well to check that the constants generated are in the right precision format?

[neil.hickey]

Added a CodeGen test as well as fixed an issue with vararg type promotion

[Anastasia]

lib/Sema/SemaExpr.cpp

832 ↗

(On Diff #70527)

This should go on the previous line.

837 ↗

(On Diff #70527)

Could we merge this and two lines above into one?

840 ↗

(On Diff #70527)

I think the formatting is not right here.

Could you change to:

} else {

test/CodeGenOpenCL/fpmath.cl

28 ↗

(On Diff #70527)

Could you please add a check that double is generated in either CL2.0 or fp64 mode.

[neil.hickey]

Modifying formating to match LLVM style. Adding new test to confirm that the vararg for printf is cast up to a double is allowable.

[neil.hickey]

lib/Sema/SemaExpr.cpp

837 ↗

(On Diff #70527)

This was the result when I ran this through clang-format so I'd rather keep it as it is.

[Anastasia]

LGTM! Thanks!

[neil.hickey]

Commit merged to trunk

[neil.hickey]

committed @ 281714

[neil.hickey]

There was a bug whereby an implicitcast was being applied from float to float, this caused issues later on in builin processing which caused an assertion. This changed patch removes the duplicate, superfluous, cast.

[yaxunl]

lib/Sema/SemaExpr.cpp

3431 ↗

(On Diff #71837)

This check

(getLangOpts().OpenCLVersion >= 120 &&
                  Context.getTargetInfo()
                      .getSupportedOpenCLOpts()
                      .cl_khr_fp64)

is redundant since for CL 1.2 and above getOpenCLOptions().cl_khr_fp64 is set to be true by default.

[neil.hickey]

lib/Sema/SemaExpr.cpp

3431 ↗

(On Diff #71837)

This is getSupportedOpenCLOpts(). Some hardware may not support doubles

[yaxunl]

lib/Sema/SemaExpr.cpp

3431 ↗

(On Diff #71837)

In Sema::Initialize(), there is code to initialize enabled extensions:

  // Initialize predefined OpenCL types and supported optional core features.
  if (getLangOpts().OpenCL) {
#define OPENCLEXT(Ext) \
     if (Context.getTargetInfo().getSupportedOpenCLOpts().is_##Ext##_supported_core( \
         getLangOpts().OpenCLVersion)) \
       getOpenCLOptions().Ext = 1;
#include "clang/Basic/OpenCLExtensions.def"

is_##Ext##_supported_core accounts for two factors: 1. whether the target supports the extension; 2. whether the extension has become OpenCL core featrue.

Since getOpenCLOptions().cl_khr_fp64 is initialized with the same value as in the mentioned check, the check is redundant.

[neil.hickey]

Sorry for the delay. It looks like the code to handle extensions was changed since Neil Henning added the check against opencl 1.2. Perhaps the best approach is just to remove the check.

[yaxunl]

test/SemaOpenCL/extensions.cl

28 ↗

(On Diff #76587)

expected-no-diagnostics applies to the whole file. better move to the beginning of the file.

otherwise LGTM. Thanks!

[neil.hickey]

Moving location of no-diagnostics statement to beginning of file

[svenvh]

lib/Sema/SemaExpr.cpp

3431 ↗

(On Diff #71837)

Your new patch no longer does version checks, so there is no need to mention it in the comments anymore I'd say.

[neil.hickey]

Improving now confusing comment.

[svenvh]

LGTM!

[neil.hickey]

Fixes to tests and removal of incorrect check to stop float to float casts if types match. This was still needed as it was an lvalue to rvalue cast. Added extra code in SemaChecking to allow a float to float cast to appear and be handled.

[svenvh]

lib/Sema/SemaChecking.cpp

3732 ↗

(On Diff #77961)

The comment should mention float -> float now as well.

lib/Sema/SemaExpr.cpp

712 ↗

(On Diff #77961)

You can change this back to what it was before your first commit (which also fixes the indentation).

[neil.hickey]

Fixing indentation and improving comments.

[svenvh]

Thanks, lgtm!

cfe/trunk/lib/Sema/SemaChecking.cpp

if (OrigArg->isTypeDependent())
     return false;
 
   // This operation requires a non-_Complex floating-point number.
   if (!OrigArg->getType()->isRealFloatingType())
     return Diag(OrigArg->getLocStart(),
                 diag::err_typecheck_call_invalid_unary_fp)
       << OrigArg->getType() << OrigArg->getSourceRange();
 
-  // If this is an implicit conversion from float -> double, remove it.
+  // If this is an implicit conversion from float -> float or double, remove it.
   if (ImplicitCastExpr *Cast = dyn_cast<ImplicitCastExpr>(OrigArg)) {
     Expr *CastArg = Cast->getSubExpr();
     if (CastArg->getType()->isSpecificBuiltinType(BuiltinType::Float)) {
-      assert(Cast->getType()->isSpecificBuiltinType(BuiltinType::Double) &&
-             "promotion from float to double is the only expected cast here");
+        assert((Cast->getType()->isSpecificBuiltinType(BuiltinType::Double) ||
+                Cast->getType()->isSpecificBuiltinType(BuiltinType::Float)) &&
+             "promotion from float to either float or double is the only expected cast here");
       Cast->setSubExpr(nullptr);
       TheCall->setArg(NumArgs-1, CastArg);
     }
   }
   
   return false;
 }
 

cfe/trunk/lib/Sema/SemaExpr.cpp

ExprResult Sema::DefaultArgumentPromotion(Expr *E) {
   if (Res.isInvalid())
     return ExprError();
   E = Res.get();
 
   // If this is a 'float' or '__fp16' (CVR qualified or typedef) promote to
   // double.
   const BuiltinType *BTy = Ty->getAs<BuiltinType>();
   if (BTy && (BTy->getKind() == BuiltinType::Half ||
-              BTy->getKind() == BuiltinType::Float))
+              BTy->getKind() == BuiltinType::Float)) {
+    if (getLangOpts().OpenCL &&
+        !(getOpenCLOptions().cl_khr_fp64)) {
+        if (BTy->getKind() == BuiltinType::Half) {
+            E = ImpCastExprToType(E, Context.FloatTy, CK_FloatingCast).get();
+        }
+    } else {
       E = ImpCastExprToType(E, Context.DoubleTy, CK_FloatingCast).get();
+    }
+  }
 
   // C++ performs lvalue-to-rvalue conversion as a default argument
   // promotion, even on class types, but note:
   //   C++11 [conv.lval]p2:
   //     When an lvalue-to-rvalue conversion occurs in an unevaluated
   //     operand or a subexpression thereof the value contained in the
   //     referenced object is not accessed. Otherwise, if the glvalue
   //     has a class type, the conversion copy-initializes a temporary
else if (Literal.isFloat128)
       Ty = Context.Float128Ty;
     else
       Ty = Context.DoubleTy;
 
     Res = BuildFloatingLiteral(*this, Literal, Ty, Tok.getLocation());
 
     if (Ty == Context.DoubleTy) {
       if (getLangOpts().SinglePrecisionConstants) {
+        const BuiltinType *BTy = Ty->getAs<BuiltinType>();
+        if (BTy->getKind() != BuiltinType::Float) {
           Res = ImpCastExprToType(Res, Context.FloatTy, CK_FloatingCast).get();
+        }
       } else if (getLangOpts().OpenCL &&
-                 !((getLangOpts().OpenCLVersion >= 120) ||
-                   getOpenCLOptions().cl_khr_fp64)) {
+                 !(getOpenCLOptions().cl_khr_fp64)) {
+        // Impose single-precision float type when cl_khr_fp64 is not enabled.
         Diag(Tok.getLocation(), diag::warn_double_const_requires_fp64);
         Res = ImpCastExprToType(Res, Context.FloatTy, CK_FloatingCast).get();
       }
     }
   } else if (!Literal.isIntegerLiteral()) {
     return ExprError();
   } else {
     QualType Ty;

cfe/trunk/lib/Sema/SemaType.cpp

static QualType ConvertDeclSpecToType(TypeProcessingState &state) {
   case DeclSpec::TST_float: Result = Context.FloatTy; break;
   case DeclSpec::TST_double:
     if (DS.getTypeSpecWidth() == DeclSpec::TSW_long)
       Result = Context.LongDoubleTy;
     else
       Result = Context.DoubleTy;
 
     if (S.getLangOpts().OpenCL &&
-        !((S.getLangOpts().OpenCLVersion >= 120) ||
-          S.getOpenCLOptions().cl_khr_fp64)) {
+        !(S.getOpenCLOptions().cl_khr_fp64)) {
       S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_requires_extension)
           << Result << "cl_khr_fp64";
       declarator.setInvalidType(true);
     }
     break;
   case DeclSpec::TST_float128:
     if (!S.Context.getTargetInfo().hasFloat128Type())
       S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported)

cfe/trunk/test/CodeGenOpenCL/fpmath.cl

 // RUN: %clang_cc1 %s -emit-llvm -o - -triple spir-unknown-unknown | FileCheck --check-prefix=CHECK --check-prefix=NODIVOPT %s
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple spir-unknown-unknown -cl-fp32-correctly-rounded-divide-sqrt | FileCheck --check-prefix=CHECK --check-prefix=DIVOPT %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -DNOFP64 -cl-std=CL1.2 -triple r600-unknown-unknown -target-cpu r600 -pedantic | FileCheck --check-prefix=CHECK-FLT %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -DFP64 -cl-std=CL1.2 -pedantic | FileCheck --check-prefix=CHECK-DBL %s
 
 typedef __attribute__(( ext_vector_type(4) )) float float4;
 
 float spscalardiv(float a, float b) {
   // CHECK: @spscalardiv
   // CHECK: #[[ATTR:[0-9]+]]
   // CHECK: fdiv{{.*}},
   // NODIVOPT: !fpmath ![[MD:[0-9]+]]
   // DIVOPT-NOT: !fpmath ![[MD:[0-9]+]]
   return a / b;
 }
 
 float4 spvectordiv(float4 a, float4 b) {
   // CHECK: @spvectordiv
   // CHECK: #[[ATTR]]
   // CHECK: fdiv{{.*}},
   // NODIVOPT: !fpmath ![[MD]]
   // DIVOPT-NOT: !fpmath ![[MD]]
   return a / b;
 }
 
-#pragma OPENCL EXTENSION cl_khr_fp64 : enable
+#if __OPENCL_C_VERSION__ >=120
+void printf(constant char* fmt, ...);
+
+void testdbllit(long *val) {
+  // CHECK-FLT: float 2.000000e+01
+  // CHECK-DBL: double 2.000000e+01
+  printf("%f", 20.0);
+}
 
+#endif
+
+#ifndef NOFP64
+#pragma OPENCL EXTENSION cl_khr_fp64 : enable
 double dpscalardiv(double a, double b) {
   // CHECK: @dpscalardiv
   // CHECK: #[[ATTR]]
   // CHECK-NOT: !fpmath
   return a / b;
 }
+#endif
 
 // CHECK: attributes #[[ATTR]] = {
 // NODIVOPT: "correctly-rounded-divide-sqrt-fp-math"="false"
 // DIVOPT: "correctly-rounded-divide-sqrt-fp-math"="true"
 // CHECK: }
 // NODIVOPT: ![[MD]] = !{float 2.500000e+00}

cfe/trunk/test/SemaOpenCL/extensions.cl

 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL1.1
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL1.2 -DFP64
 
 // Test with a target not supporting fp64.
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -DNOFP64 -DNOFP16
 
 // Test with some extensions enabled or disabled by cmd-line args
 //
 // Target does not support fp64 and fp16 - override it
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+cl_khr_fp64,+cl_khr_fp16
 //
 // Disable or enable all extensions
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -DNOFP64 -DNOFP16
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+all
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+all,-cl_khr_fp64 -DNOFP64
 // RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=-all,+cl_khr_fp64 -DNOFP16
 //
 // Concatenating
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-cl_khr_fp64 -cl-ext=+cl_khr_fp64
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-cl_khr_fp64,+cl_khr_fp64
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64 -cl-ext=+cl_khr_fp16 -cl-ext=-cl_khr_fp64 -DNOFP64
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64,-cl_khr_fp64,+cl_khr_fp16 -DNOFP64
 
 
 
+#ifdef FP64
+// expected-no-diagnostics
+#endif
+
+#if __OPENCL_C_VERSION__ < 120
 void f1(double da) { // expected-error {{type 'double' requires cl_khr_fp64 extension}}
   double d; // expected-error {{type 'double' requires cl_khr_fp64 extension}}
   (void) 1.0; // expected-warning {{double precision constant requires cl_khr_fp64}}
 }
+#endif
 
 #pragma OPENCL EXTENSION cl_khr_fp64 : enable
 #ifdef NOFP64
 // expected-warning@-2{{unsupported OpenCL extension 'cl_khr_fp64' - ignoring}}
 #endif
 
 #pragma OPENCL EXTENSION cl_khr_fp16 : enable
 #ifdef NOFP16
 // expected-warning@-2{{unsupported OpenCL extension 'cl_khr_fp16' - ignoring}}
 #endif
 
 void f2(void) {
   double d;
 #ifdef NOFP64
 // expected-error@-2{{use of type 'double' requires cl_khr_fp64 extension to be enabled}}
 #endif
 
   (void) 1.0;
+
 #ifdef NOFP64
-// expected-warning@-2{{double precision constant requires cl_khr_fp64, casting to single precision}}
+// expected-warning@-3{{double precision constant requires cl_khr_fp64, casting to single precision}}
 #endif
 }
 
 #pragma OPENCL EXTENSION cl_khr_fp64 : disable
 #ifdef NOFP64
 // expected-warning@-2{{unsupported OpenCL extension 'cl_khr_fp64' - ignoring}}
 #endif
 
+#if __OPENCL_C_VERSION__ < 120
 void f3(void) {
   double d; // expected-error {{type 'double' requires cl_khr_fp64 extension}}
 }
+#endif