[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG13ea238b1e1d: [OpenCL] Allow use of double type without 
extension pragma. (authored by Anastasia).
Herald added a subscriber: ldrumm.
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D100980?vs=341833=344360#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Misc/warning-flags.c
  clang/test/SemaOpenCL/extensions.cl

Index: clang/test/SemaOpenCL/extensions.cl
===
--- clang/test/SemaOpenCL/extensions.cl
+++ clang/test/SemaOpenCL/extensions.cl
@@ -1,5 +1,6 @@
 // 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 -fsyntax-only -cl-std=CL1.1 -DNOPEDANTIC
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL1.2 -DFP64
 
 // Test with a target not supporting fp64.
@@ -43,8 +44,20 @@
 #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 f1(double da) {
+#ifdef NOFP64
+// expected-error@-2 {{type 'double' requires cl_khr_fp64 support}}
+#elif !defined(NOPEDANTIC)
+// expected-warning@-4{{Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is supported}}
+#endif
+  double d;
+#ifdef NOFP64
+// expected-error@-2 {{type 'double' requires cl_khr_fp64 support}}
+#elif !defined(NOPEDANTIC)
+// expected-warning@-4{{Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is supported}}
+#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
@@ -79,13 +92,11 @@
   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;
-
 #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
 }
 
@@ -96,6 +107,11 @@
 
 #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}}
+#elif !defined(NOPEDANTIC)
+// expected-warning@-4 {{Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is supported}}
+#endif
 }
 #endif
Index: clang/test/Misc/warning-flags.c
===
--- clang/test/Misc/warning-flags.c
+++ clang/test/Misc/warning-flags.c
@@ -91,4 +91,4 @@
 
 The list of warnings in -Wpedantic should NEVER grow.
 
-CHECK: Number in -Wpedantic (not covered by other -W flags): 26
+CHECK: Number in -Wpedantic (not covered by other -W flags): 27
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -1524,6 +1524,13 @@
 break;
   case DeclSpec::TST_float:   Result = Context.FloatTy; break;
   case DeclSpec::TST_double:
+if (S.getLangOpts().OpenCL) {
+  if (!S.getOpenCLOptions().isSupported("cl_khr_fp64", S.getLangOpts()))
+S.Diag(DS.getTypeSpecTypeLoc(), diag::err_opencl_requires_extension)
+  << 0 << Context.DoubleTy << "cl_khr_fp64";
+  else if (!S.getOpenCLOptions().isAvailableOption("cl_khr_fp64", S.getLangOpts()))
+S.Diag(DS.getTypeSpecTypeLoc(), diag::ext_opencl_double_without_pragma);
+}
 if (DS.getTypeSpecWidth() == TypeSpecifierWidth::Long)
   Result = Context.LongDoubleTy;
 else
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -366,7 +366,6 @@
 "cl_khr_int64_base_atomics cl_khr_int64_extended_atomics");
 }
 
-setOpenCLExtensionForType(Context.DoubleTy, "cl_khr_fp64");
 
 #define EXT_OPAQUE_TYPE(ExtType, Id, Ext)  \
   if (getOpenCLOptions().isSupported(#Ext, 

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10006
   "use of %select{type|declaration}0 %1 requires %2 support">;
+def ext_opencl_double_without_pragma : Extension<
+  "Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is 
supported">;

azabaznov wrote:
> Anastasia wrote:
> > azabaznov wrote:
> > > nit: this can be extended to use arbitrary type and extension for other 
> > > patches which will eliminate pragmas for types
> > 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.  
> > 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10006
   "use of %select{type|declaration}0 %1 requires %2 support">;
+def ext_opencl_double_without_pragma : Extension<
+  "Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is 
supported">;

Anastasia wrote:
> azabaznov wrote:
> > nit: this can be extended to use arbitrary type and extension for other 
> > patches which will eliminate pragmas for types
> 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.  
> 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10006
   "use of %select{type|declaration}0 %1 requires %2 support">;
+def ext_opencl_double_without_pragma : Extension<
+  "Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is 
supported">;

azabaznov wrote:
> nit: this can be extended to use arbitrary type and extension for other 
> patches which will eliminate pragmas for types
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.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov accepted this revision.
azabaznov added a comment.
This revision is now accepted and ready to land.

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




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10006
   "use of %select{type|declaration}0 %1 requires %2 support">;
+def ext_opencl_double_without_pragma : Extension<
+  "Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is 
supported">;

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 341833.
Anastasia added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Misc/warning-flags.c
  clang/test/SemaOpenCL/extensions.cl

Index: clang/test/SemaOpenCL/extensions.cl
===
--- clang/test/SemaOpenCL/extensions.cl
+++ clang/test/SemaOpenCL/extensions.cl
@@ -1,5 +1,6 @@
 // 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 -fsyntax-only -cl-std=CL1.1 -DNOPEDANTIC
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL1.2 -DFP64
 
 // Test with a target not supporting fp64.
@@ -43,8 +44,20 @@
 #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 f1(double da) {
+#ifdef NOFP64
+// expected-error@-2 {{type 'double' requires cl_khr_fp64 support}}
+#elif !defined(NOPEDANTIC)
+// expected-warning@-4{{Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is supported}}
+#endif
+  double d;
+#ifdef NOFP64
+// expected-error@-2 {{type 'double' requires cl_khr_fp64 support}}
+#elif !defined(NOPEDANTIC)
+// expected-warning@-4{{Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is supported}}
+#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
@@ -79,13 +92,11 @@
   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;
-
 #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
 }
 
@@ -96,6 +107,11 @@
 
 #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}}
+#elif !defined(NOPEDANTIC)
+// expected-warning@-4 {{Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is supported}}
+#endif
 }
 #endif
Index: clang/test/Misc/warning-flags.c
===
--- clang/test/Misc/warning-flags.c
+++ clang/test/Misc/warning-flags.c
@@ -91,4 +91,4 @@
 
 The list of warnings in -Wpedantic should NEVER grow.
 
-CHECK: Number in -Wpedantic (not covered by other -W flags): 26
+CHECK: Number in -Wpedantic (not covered by other -W flags): 27
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -1524,6 +1524,13 @@
 break;
   case DeclSpec::TST_float:   Result = Context.FloatTy; break;
   case DeclSpec::TST_double:
+if (S.getLangOpts().OpenCL) {
+  if (!S.getOpenCLOptions().isSupported("cl_khr_fp64", S.getLangOpts()))
+S.Diag(DS.getTypeSpecTypeLoc(), diag::err_opencl_requires_extension)
+  << 0 << Context.DoubleTy << "cl_khr_fp64";
+  else if (!S.getOpenCLOptions().isAvailableOption("cl_khr_fp64", S.getLangOpts()))
+S.Diag(DS.getTypeSpecTypeLoc(), diag::ext_opencl_double_without_pragma);
+}
 if (DS.getTypeSpecWidth() == TypeSpecifierWidth::Long)
   Result = Context.LongDoubleTy;
 else
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -365,7 +365,6 @@
   }
 }
 
-setOpenCLExtensionForType(Context.DoubleTy, "cl_khr_fp64");
 
 #define GENERIC_IMAGE_TYPE_EXT(Type, Id, Ext) \
 setOpenCLExtensionForType(Context.Id, Ext);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10003,6 +10003,8 @@
   "invalid prototype, variadic arguments are not allowed in OpenCL">;
 def 

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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):
>
>   :6:5: error: use of type 'double' requires cl_khr_fp64 support
>   double d;
>   ^
>   1 error generated.
>
> What I suggest is to have:
>
>   :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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

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):

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

What I suggest is to have:

  :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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

> 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.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

> 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

> 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

> 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100980/new/

https://reviews.llvm.org/D100980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100980: [OpenCL] Allow use of double type without extension pragma

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: svenvh, azabaznov.
Herald added subscribers: ebevhan, yaxunl.
Anastasia requested review of this revision.

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.


https://reviews.llvm.org/D100980

Files:
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaOpenCL/extensions.cl


Index: clang/test/SemaOpenCL/extensions.cl
===
--- clang/test/SemaOpenCL/extensions.cl
+++ clang/test/SemaOpenCL/extensions.cl
@@ -43,9 +43,17 @@
 #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
 
@@ -79,13 +87,11 @@
   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
 }
 
@@ -96,6 +102,9 @@
 
 #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
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -1524,6 +1524,10 @@
 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
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -363,7 +363,6 @@
   }
 }
 
-setOpenCLExtensionForType(Context.DoubleTy, "cl_khr_fp64");
 
 #define GENERIC_IMAGE_TYPE_EXT(Type, Id, Ext) \
 setOpenCLExtensionForType(Context.Id, Ext);


Index: clang/test/SemaOpenCL/extensions.cl
===
--- clang/test/SemaOpenCL/extensions.cl
+++ clang/test/SemaOpenCL/extensions.cl
@@ -43,9 +43,17 @@
 #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
 
@@ -79,13 +87,11 @@
   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
 
-