[OpenCL] Allow use of double type without extension pragma

[Anastasia]

Summary

OpenCL specification doesn't require the pragma for the uses of double type when it is supported by the targets. The wording in the spec is as follows:

80. Only if double precision is supported. In OpenCL C 3.0 this will be indicated by the presence of the __opencl_c_fp64 feature macro.

in contrast to half type

79. Only if the cl_khr_fp16 extension is supported and has been enabled.

This is primarily due to the fact that double is a type from C99 spec where it can be used without any pragmas. This is to align the core OpenCL behavior with C.

This patch does not break backward compatibility since the extension pragma is still supported and it makes the behavior of the compiler less strict by accepting code without extra pragma statements.

[azabaznov]

Same as for https://reviews.llvm.org/D100984, cl_khr_fp64 wasn't always core and thus it requires pragma for OpenCL C < 1.2 versions.

9.3 Double Precision Floating-Point, OpenCL C 1.0 (https://www.khronos.org/registry/OpenCL/specs/opencl-1.0.pdf):

OpenCL 1.0 adds support for double precision floating-point as an optional extension. An application that wants to use double will need to include the #pragma OPENCL EXTENSION cl_khr_fp64 : enable directive before any double precision data type is declared in the kernel code.

[Anastasia]

In D100980#2708012, @azabaznov wrote:

Same as for https://reviews.llvm.org/D100984, cl_khr_fp64 wasn't always core and thus it requires pragma for OpenCL C < 1.2 versions.

9.3 Double Precision Floating-Point, OpenCL C 1.0 (https://www.khronos.org/registry/OpenCL/specs/opencl-1.0.pdf):

OpenCL 1.0 adds support for double precision floating-point as an optional extension. An application that wants to use double will need to include the #pragma OPENCL EXTENSION cl_khr_fp64 : enable directive before any double precision data type is declared in the kernel code.

Ok, we can change to check for

S.getOpenCLOptions().isAvailable("cl_khr_fp64", S.getLangOpts())

instead of isSupported but out of curiosity considering that we have failed to implement the extension pragmas anyway (see description in https://reviews.llvm.org/D100976) do you think it is valuable to keep this behavior at all? It would be like doing something partially correct but not fully correct.

The reason why I would prefer to avoid unnecessary uses of prgmas is that they are confusing and inconsistent so the less of them we have the easier it is for the developers and users of clang.

[azabaznov]

do you think it is valuable to keep this behavior at all?

As I said, I would be happy too if we remove pragma extension as it will really simplify the codebase of OpenCL in clang and the usage of optional functionality itself. Maybe we should add a diagnostic that pragma is ignored?

[Anastasia]

In D100980#2711588, @azabaznov wrote:

do you think it is valuable to keep this behavior at all?

As I said, I would be happy too if we remove pragma extension as it will really simplify the codebase of OpenCL in clang and the usage of optional functionality itself. Maybe we should add a diagnostic that pragma is ignored?

For cl_khr_fp64 I still want to keep the pragma for the other use case - to convert double literal into single-precision (https://github.com/KhronosGroup/OpenCL-Docs/issues/578). The reason why I think it could be useful is that the precision change might lead to a different result depending on the precision of the calculation. So I think pragmas could be useful to control whether double literal is single-precision or not to avoid this problem occur when code is compiled for different targets?

When we parse the pragma we don't know what it is used for in the code so it won't be possible to emit the warning conditionally.

[azabaznov]

For cl_khr_fp64 I still want to keep the pragma for the other use case - to convert double literal into single-precision (https://github.com/KhronosGroup/OpenCL-Docs/issues/578). The reason why I think it could be useful is that the precision change might lead to a different result depending on the precision of the calculation. So I think pragmas could be useful to control whether double literal is single-precision or not to avoid this problem occur when code is compiled for different targets?

Yeah, then we should use S.getOpenCLOptions().isAvailableOption("cl_khr_fp64", S.getLangOpts()) to check for enabled pragma. And do I understand correctly that the pragma is not needed in OpenCL C 1.2+ since extension became core?

[Anastasia]

In D100980#2711963, @azabaznov wrote:

For cl_khr_fp64 I still want to keep the pragma for the other use case - to convert double literal into single-precision (https://github.com/KhronosGroup/OpenCL-Docs/issues/578). The reason why I think it could be useful is that the precision change might lead to a different result depending on the precision of the calculation. So I think pragmas could be useful to control whether double literal is single-precision or not to avoid this problem occur when code is compiled for different targets?

Yeah, then we should use S.getOpenCLOptions().isAvailableOption("cl_khr_fp64", S.getLangOpts()) to check for enabled pragma.

Just to be clear, we already use isAvailableOption in other cases, for example this:
https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html#l00816

My idea was to simplify only one particular case when the data types are being used because I don't find the pragma useful for it and it is also inconsistent.

We could of course keep it just for this particular case of doubles, but even half is allowed in certain circumstances without the pragma and it is still an extension.
https://godbolt.org/z/K34sP81nx

So I personally don't see a big value to keep it just for the particular case of doubles.

And do I understand correctly that the pragma is not needed in OpenCL C 1.2+ since extension became core?

To be honest I don't really know. In the unified spec there is nothing about the pragmas. For now I just want to address the use cases that don't cause backward compatibility issues.

[azabaznov]

We could of course keep it just for this particular case of doubles, but even half is allowed in certain circumstances without the pragma and it is still an extension. https://godbolt.org/z/K34sP81nx

I am confused again... after looking into OpenCL C 1.0, 9.10 Half Floating-Point: it says that pragma is required too. So is it a bug in clang implementation are do you prefer keep existing functionality for double?

[Anastasia]

In D100980#2712534, @azabaznov wrote:

We could of course keep it just for this particular case of doubles, but even half is allowed in certain circumstances without the pragma and it is still an extension. https://godbolt.org/z/K34sP81nx

I am confused again... after looking into OpenCL C 1.0, 9.10 Half Floating-Point: it says that pragma is required too. So is it a bug in clang implementation are do you prefer keep existing functionality for double?

I am not sure, to be honest I personally think the extension pragma is a spec failure as it is not specified properly or to allow reasonable implementation. And since nobody really understands the original intent it was not possible to provide any reasonable implementation either.

For example, in case of half the extension spec said this:

An application that wants to use half and halfn types will need to include the #pragma OPENCL EXTENSION cl_khr_fp16 : enabledirective.

While main spec has:

The half data type can only be used to declare a pointer to a buffer that contains half values.

half is not supported as half can be used as a storage format only and is not a data type on which floating-point arithmetic can be performed

It has no pragma in any examples.

I can't really tell what the pragma was intended for. We have this separate spec bug for it btw: https://github.com/KhronosGroup/OpenCL-Docs/issues/21

I didn't look at fp16 in my cleanup yet as it doesn't have a similar pattern with any other extension and it is more complicated in my opinion.

Anyway since there is not clear benefit that can be found now for the pragma I think we should minimize its use as much as possible.

[azabaznov]

I am not sure, to be honest I personally think the extension pragma is a spec failure as it is not specified properly or to allow reasonable implementation

Unfortunately it's already there :(

Anyway since there is not clear benefit that can be found now for the pragma I think we should minimize its use as much as possible.

I personally agree, but I believe in order to go forward this patch should introduce diagnostics to commit the introduction of new functionality, I'm not sure what exactly it should be. I'm thinking of some pedantic one such as: "pragma enable is no longer required for type usage". What do you think?

[Anastasia]

In D100980#2719043, @azabaznov wrote:

I am not sure, to be honest I personally think the extension pragma is a spec failure as it is not specified properly or to allow reasonable implementation

Unfortunately it's already there :(

Anyway since there is not clear benefit that can be found now for the pragma I think we should minimize its use as much as possible.

I personally agree, but I believe in order to go forward this patch should introduce diagnostics to commit the introduction of new functionality, I'm not sure what exactly it should be. I'm thinking of some pedantic one such as: "pragma enable is no longer required for type usage". What do you think?

Well I don't see how we can do this now because we still use the pragma for other cases. I only remove one use of it for declaring the types but there are others for example for literal conversion:
https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html#l00816

When the pragma is parsed we can't know why it is in the code to be able to issue any warning.

If we remove all uses of pragma for cl_khr_fp64 we could do something like https://reviews.llvm.org/D91534. But we can't do this yet.

My current clean-up only covers one particular case of requiring pragma for declaring a type because I don't see why it is useful and it is safe to relax as it doesn't cause kernels to compile differently. We can look at other cases later but they are relatively infrequent and don't cause lots of maintenance.

[azabaznov]

When the pragma is parsed we can't know why it is in the code to be able to issue any warning.

I mean diagnose once when, for example in your particular case, double type is parsed. Does it require much effort? I think this warning might be useful for developers who already rely on pragma usage in their kernels.

[Anastasia]

In D100980#2719196, @azabaznov wrote:

When the pragma is parsed we can't know why it is in the code to be able to issue any warning.

I mean diagnose once when, for example in your particular case, double type is parsed. Does it require much effort? I think this warning might be useful for developers who already rely on pragma usage in their kernels.

I am not sure I understand your suggestion. Could you show an example perhaps?

[azabaznov]

In D100980#2719322, @Anastasia wrote:

In D100980#2719196, @azabaznov wrote:

When the pragma is parsed we can't know why it is in the code to be able to issue any warning.

I mean diagnose once when, for example in your particular case, double type is parsed. Does it require much effort? I think this warning might be useful for developers who already rely on pragma usage in their kernels.

I am not sure I understand your suggestion. Could you show an example perhaps?

All right. Currently, what we do have for OpenCL C < 1.2 (https://godbolt.org/z/rjYWMj7v1):

<source>:6:5: error: use of type 'double' requires cl_khr_fp64 support
    double d;
    ^
1 error generated.

What I suggest is to have:

<source>:6:5: warning: pragma enable is no longer required for use of type 'double'
    double d;
    ^
1 warning generated.

We can issue the warning if certain flag is provided for example. Does it make sense?

[Anastasia]

In D100980#2722112, @azabaznov wrote:

In D100980#2719322, @Anastasia wrote:

In D100980#2719196, @azabaznov wrote:

When the pragma is parsed we can't know why it is in the code to be able to issue any warning.

I mean diagnose once when, for example in your particular case, double type is parsed. Does it require much effort? I think this warning might be useful for developers who already rely on pragma usage in their kernels.

I am not sure I understand your suggestion. Could you show an example perhaps?

All right. Currently, what we do have for OpenCL C < 1.2 (https://godbolt.org/z/rjYWMj7v1):

<source>:6:5: error: use of type 'double' requires cl_khr_fp64 support
    double d;
    ^
1 error generated.

What I suggest is to have:

<source>:6:5: warning: pragma enable is no longer required for use of type 'double'
    double d;
    ^
1 warning generated.

We can issue the warning if certain flag is provided for example. Does it make sense?

Not sure what do we want to achieve with this? Do you want to point out that the code might be somehow less portable let's say between clang revisions, etc? I think we could do it as it should not be too complicated but adds a bit extra complexity into the command line interface.

[azabaznov]

Not sure what do we want to achieve with this? Do you want to point out that the code might be somehow less portable let's say between clang revisions, etc?

My main worry is that you are changing the behaviour here: kernels which fail to compile will compile successfully. I suggest not to do it silently but issue a warning/note instead.

[Anastasia]

In D100980#2722207, @azabaznov wrote:

Not sure what do we want to achieve with this? Do you want to point out that the code might be somehow less portable let's say between clang revisions, etc?

My main worry is that you are changing the behaviour here: kernels which fail to compile will compile successfully. I suggest not to do it silently but issue a warning/note instead.

Ok, this should technically not be an issue as it does not break backward compatibility but it could still be useful for the portability reasons especially in the transition period while we still accept but deprecate pragmas. Perhaps we could even use such a flag elsewhere for similar purposes.

However, what I don't like about it is that we will still need to check for whether the pragma is enabled in the compiler source code so it won't be simpler. Anyway I guess the best way is to prepare a patch and then we can take a look and decide whether we can go ahead with it or stick to an old solution with pragma for the time being.

[Anastasia]

• Added a pedantic warning about use of double even if the extension is in disabled state wrt pragma.

[azabaznov]

Thanks! Looks good to me in general. See a comment.

clang/include/clang/Basic/DiagnosticSemaKinds.td

10006 ↗

(On Diff #341833)

nit: this can be extended to use arbitrary type and extension for other patches which will eliminate pragmas for types

[Anastasia]

clang/include/clang/Basic/DiagnosticSemaKinds.td

10006 ↗

(On Diff #341833)

Well, actually I am not sure we should use it elsewhere because I don't think it really applies universally.
The situation for doubles seems to be that the older specs were intructing to use the pragma explicitly.

For the future work we should just avoid adding or using pragma at all unless it brings beneficial functionality.

[azabaznov]

clang/include/clang/Basic/DiagnosticSemaKinds.td

10006 ↗

(On Diff #341833)

Well, actually I am not sure we should use it elsewhere because I don't think it really applies universally.
The situation for doubles seems to be that the older specs were intructing to use the pragma explicitly.

The same applies for atomic types, for example (https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#_footnoteref_55):

If the device address space is 64-bits, the data types atomic_intptr_t, atomic_uintptr_t, atomic_size_t and atomic_ptrdiff_t are supported if the cl_khr_int64_base_atomics and cl_khr_int64_extended_atomics extensions are supported and have been enabled.

It seems for me that simplifying the implementation in a way that enabling pragma is not necessary is fine if such warning is diagnosed for atomic types and half type as well. Alternatively, maybe the spec should be fixed by adding __opencl_c_fp16 and etc. as optional core features?

[Anastasia]

clang/include/clang/Basic/DiagnosticSemaKinds.td

10006 ↗

(On Diff #341833)

Ok, this is a different issues in my view:
a. It doesn't explicitly mention the pragma although I assume it is implied.
b. It assumes the pragmas could load and unload the extensions but it has not been implemented correctly. As a matter of fact I am removing the requirements for the pragma on atomics completely in https://reviews.llvm.org/D100976 because I see no value in implementing it half way ending up in incorrect and inconveninent functionality.

Double support is different to atomics because double is actually a reserved identifier so disabling it actually works as expected.

clang/lib/Sema/Sema.cpp

if (getLangOpts().OpenCLCPlusPlus || getLangOpts().OpenCLVersion >= 200) {
         addImplicitTypedef("atomic_ulong", AtomicULongT);
 
         if (Context.getTypeSize(Context.getSizeType()) == 64) {
           AddPointerSizeDependentTypes();
         }
       }
     }
 
-    setOpenCLExtensionForType(Context.DoubleTy, "cl_khr_fp64");
 
 #define GENERIC_IMAGE_TYPE_EXT(Type, Id, Ext) \
     setOpenCLExtensionForType(Context.Id, Ext);
 #include "clang/Basic/OpenCLImageTypes.def"
 #define EXT_OPAQUE_TYPE(ExtType, Id, Ext)                                      \
   if (getOpenCLOptions().isSupported(#Ext, getLangOpts())) {                   \
     addImplicitTypedef(#ExtType, Context.Id##Ty);                              \
     setOpenCLExtensionForType(Context.Id##Ty, #Ext);                           \

clang/lib/Sema/SemaType.cpp

static QualType ConvertDeclSpecToType(TypeProcessingState &state) {
   case DeclSpec::TST_BFloat16:
     if (!S.Context.getTargetInfo().hasBFloat16Type())
       S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported)
         << "__bf16";
     Result = Context.BFloat16Ty;
     break;
   case DeclSpec::TST_float:   Result = Context.FloatTy; break;
   case DeclSpec::TST_double:
+    if (S.getLangOpts().OpenCL &&
+        !S.getOpenCLOptions().isSupported("cl_khr_fp64", S.getLangOpts()))
+      S.Diag(DS.getTypeSpecTypeLoc(), diag::err_opencl_requires_extension)
+          << 0 << Context.DoubleTy << "cl_khr_fp64";
     if (DS.getTypeSpecWidth() == TypeSpecifierWidth::Long)
       Result = Context.LongDoubleTy;
     else
       Result = Context.DoubleTy;
     break;
   case DeclSpec::TST_float128:
     if (!S.Context.getTargetInfo().hasFloat128Type() &&
         !S.getLangOpts().SYCLIsDevice &&

clang/test/SemaOpenCL/extensions.cl

 // expected-no-diagnostics
 #endif
 
 #ifdef __OPENCL_CPP_VERSION__
 // expected-no-diagnostics
 #endif
 
 #if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 120)
-void f1(double da) { // expected-error {{type 'double' requires cl_khr_fp64 support}}
-  double d; // expected-error {{type 'double' requires cl_khr_fp64 support}}
-  (void) 1.0; // expected-warning {{double precision constant requires cl_khr_fp64}}
+void f1(double da) {
+#ifdef NOFP64
+// expected-error@-2 {{type 'double' requires cl_khr_fp64 support}}
+#endif
+  double d;
+#ifdef NOFP64
+// expected-error@-2 {{type 'double' requires cl_khr_fp64 support}}
+#endif
+  // FIXME: this diagnostic depends on the extension pragma in the earlier versions.
+  // There is no indication that this behavior is expected.
+  (void)1.0; // expected-warning {{double precision constant requires cl_khr_fp64}}
 }
 #endif
 
 #ifndef _OPENCL_H_
 int isnan(float x) {
     return __builtin_isnan(x);
 }
 

 #ifdef NOFP64
 // expected-error@-2{{use of type 'double' requires cl_khr_fp64 support}}
 #endif
 
   typedef double double4 __attribute__((ext_vector_type(4)));
   double4 d4 = {0.0f, 2.0f, 3.0f, 1.0f};
 #ifdef NOFP64
 // expected-error@-3 {{use of type 'double' requires cl_khr_fp64 support}}
-// expected-error@-3 {{use of type 'double4' (vector of 4 'double' values) requires cl_khr_fp64 support}}
 #endif
 
-  (void) 1.0;
-
+  (void)1.0;
 #ifdef NOFP64
-// expected-warning@-3{{double precision constant requires cl_khr_fp64, casting to single precision}}
+// expected-warning@-2{{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 (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 120)
 void f3(void) {
-  double d; // expected-error {{type 'double' requires cl_khr_fp64 support}}
+  double d;
+#ifdef NOFP64
+// expected-error@-2 {{type 'double' requires cl_khr_fp64 support}}
+#endif
 }
 #endif