[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

835–848

This should go on the previous line.

840

Could we merge this and two lines above into one?

843

I think the formatting is not right here.

Could you change to:

} else {

test/CodeGenOpenCL/fpmath.cl

31

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

840

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

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

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

[yaxunl]

lib/Sema/SemaExpr.cpp

3431

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

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

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

723

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!

lib/Sema/SemaExpr.cpp

ExprResult Sema::DefaultLvalueConversion(Expr *E) {
 
   UpdateMarkingForLValueToRValue(E);
   
   // Loading a __weak object implicitly retains the value, so we need a cleanup to 
   // balance that.
   if (getLangOpts().ObjCAutoRefCount &&
       E->getType().getObjCLifetime() == Qualifiers::OCL_Weak)
     Cleanup.setExprNeedsCleanups(true);
   
-  ExprResult Res = ImplicitCastExpr::Create(Context, T, CK_LValueToRValue, E,
-                                            nullptr, VK_RValue);
+  ExprResult Res = E;
+
+  if ( T != E->getType()) {
+    Res = ImplicitCastExpr::Create(Context, T, CK_LValueToRValue, E,
+  }
 
   // C11 6.3.2.1p2:
   //   ... if the lvalue has atomic type, the value has the non-atomic version 
   //   of the type of the lvalue ...
   if (const AtomicType *Atomic = T->getAs<AtomicType>()) {
     T = Atomic->getValueType().getUnqualifiedType();
     Res = ImplicitCastExpr::Create(Context, T, CK_AtomicToNonAtomic, Res.get(),
                                    nullptr, VK_RValue);
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 &&
+        !((getLangOpts().OpenCLVersion >= 120 &&
+           Context.getTargetInfo()
+               .getSupportedOpenCLOpts()
+               .cl_khr_fp64) ||
+          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) ||
+                 !((getLangOpts().OpenCLVersion >= 120 &&
+                    Context.getTargetInfo()
+                        .getSupportedOpenCLOpts()
+                        .cl_khr_fp64) ||
                    getOpenCLOptions().cl_khr_fp64)) {
+        // Impose single-precision float type when:
+        //  - in CL 1.2 or above and cl_khr_fp64 is not supported, or
+        //  - in CL 1.1 or below and 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;

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.getLangOpts().OpenCLVersion >= 120 
+           && S.Context.getTargetInfo().getSupportedOpenCLOpts().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())

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.1 -triple r600-unknown-unknown -target-cpu r600 -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;
 }
 
+void printf(constant char* fmt, ...);
+
+#ifndef NOFP64
 #pragma OPENCL EXTENSION cl_khr_fp64 : enable
+#endif
+void testdbllit(long *val) {
+  // CHECK-DBL: float 2.000000e+01
+  // CHECK: double 2.000000e+01
+  printf("%f", 20.0);
+}
 
+#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}

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
 
+#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
 
 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 FP64
+// expected-no-diagnostics
+#endif
+
 #ifdef NOFP64
-// expected-warning@-2{{double precision constant requires cl_khr_fp64, casting to single precision}}
+// expected-warning@-6{{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