Re: [PATCH] D21698: [OpenCL] Allow disabling types and declarations associated with extensions

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D21698#546733, @yaxunl wrote:

> In https://reviews.llvm.org/D21698#540237, @Anastasia wrote:
>
> > I have made an experiment with a simple kernel:
> >
> >   void foo1(void);
> >   void foo2(void);
> >   void foo3(void);
> >   void foo4(void);
> >   void foo5(void);
> >   void foo6(void);
> >   void foo7(void);
> >   void foo8(void);
> >   void foo9(void);
> >   void foo10(void);
> >  
> >   void test(){
> > foo1();
> > foo2();
> > foo3();
> > foo4();
> > foo5();
> > foo6();
> > foo7();
> > foo8();
> > foo9();
> > foo10();
> >   }
> >   
> >
> > I am using time utility of linux to measure the compile time running Clang 
> > in CL2.0 mode and average over 100 samples. It shows me around 7% overhead 
> > with your approach.
>
>
> Since the program is very small, it is difficult to measure the compilation 
> time accurately. I compile the program 1000 times and measure its time in a 
> script:
>
>$ cat run.sh
>run() {
>i=1
>while [[ $i -le 1000 ]]; do
>  ./$1 -cc1 -emit-llvm tmp.cl
>  i=$((i+1))
>done
>}
>   
>   time -p run $1
>
>
>
> Even so, I found large variations in the measured compilation time. I ran the 
> script 10 times for clang before my change and I got the real time
>
>   real 8.96
>   real 9.01
>   real 8.99
>   real 9.07
>   real 9.03
>   real 8.99
>   real 9.03
>   real 9.01
>   real 8.99
>   real 9.01
>  
>
>
> and the average time is 9.009s.
>
> For clang after my change, I got
>
>   real 9.06
>   real 9.09
>   real 9.10
>   real 9.03
>   real 9.05
>   real 9.17
>   real 9.08
>   real 9.08
>   real 9.07
>   real 9.08
>  
>
>
> And the average time is 9.081s.
>
> The increase of compilation time is 0.8%. Considering this program consists 
> mostly of function declarations, which emphasized the cost of evaluating 
> disabled function declarations unrealistically. In real programs this 
> increment in compilation time should be even smaller.
>
> Since the increment of compilation time is less than 1% even for exaggerated 
> cases, I think it is reasonable to accept the cost for the improved 
> diagnostics for extensions.
>
> If there are concerns that the newly added diagnostics may break applications 
> which use builtin functions associated with an extension without enabling it, 
> we can make the diagnostic msg a warning which can be turned off.


Ok. Sure. I will profile a few more benchmarks and let you know.


https://reviews.llvm.org/D21698



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

Re: [PATCH] D21698: [OpenCL] Allow disabling types and declarations associated with extensions

[Yaxun Liu via cfe-commits]

yaxunl added a comment.

In https://reviews.llvm.org/D21698#540237, @Anastasia wrote:

> I have made an experiment with a simple kernel:
>
>   void foo1(void);
>   void foo2(void);
>   void foo3(void);
>   void foo4(void);
>   void foo5(void);
>   void foo6(void);
>   void foo7(void);
>   void foo8(void);
>   void foo9(void);
>   void foo10(void);
>  
>   void test(){
> foo1();
> foo2();
> foo3();
> foo4();
> foo5();
> foo6();
> foo7();
> foo8();
> foo9();
> foo10();
>   }
>   
>
> I am using time utility of linux to measure the compile time running Clang in 
> CL2.0 mode and average over 100 samples. It shows me around 7% overhead with 
> your approach.


Since the program is very small, it is difficult to measure the compilation 
time accurately. I compile the program 1000 times and measure its time in a 
script:

   $ cat run.sh
   run() {
   i=1
   while [[ $i -le 1000 ]]; do
 ./$1 -cc1 -emit-llvm tmp.cl
 i=$((i+1))
   done
   }
   
  time -p run $1


Even so, I found large variations in the measured compilation time. I ran the 
script 10 times for clang before my change and I got the real time

  real 8.96
  real 9.01
  real 8.99
  real 9.07
  real 9.03
  real 8.99
  real 9.03
  real 9.01
  real 8.99
  real 9.01

and the average time is 9.009s.

For clang after my change, I got

  real 9.06
  real 9.09
  real 9.10
  real 9.03
  real 9.05
  real 9.17
  real 9.08
  real 9.08
  real 9.07
  real 9.08

And the average time is 9.081s.

The increase of compilation time is 0.8%. Considering this program consists 
mostly of function declarations, which emphasized the cost of evaluating 
disabled function declarations unrealistically. In real programs this increment 
in compilation time should be even smaller.

Since the increment of compilation time is less than 1% even for exaggerated 
cases, I think it is reasonable to accept the cost for the improved diagnostics 
for extensions.

If there are concerns that the newly added diagnostics may break applications 
which use builtin functions associated with an extension without enabling it, 
we can make the diagnostic msg a warning which can be turned off.


https://reviews.llvm.org/D21698



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

Re: [PATCH] D21698: [OpenCL] Allow disabling types and declarations associated with extensions

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

I have made an experiment with a simple kernel:

  void foo1(void);
  void foo2(void);
  void foo3(void);
  void foo4(void);
  void foo5(void);
  void foo6(void);
  void foo7(void);
  void foo8(void);
  void foo9(void);
  void foo10(void);
  
  void test(){
foo1();
foo2();
foo3();
foo4();
foo5();
foo6();
foo7();
foo8();
foo9();
foo10();
  }

I am using time utility of linux to measure the compile time running Clang in 
CL2.0 mode and average over 100 samples. It shows me around 7% overhead with 
your approach.

Even thought it doesn't seem to be large overhead in this test, however it 
would be undesirable to have any regressions in compilation time in the future 
Clang releases for OpenCL basic functionality. Our target should be constantly 
to improve on that unless we have a good reason not to. But in this case I 
don't feel like the reason is strong enough. Also it would be nice to study 
more use cases to see how the compilation time scales. Compilation time might 
be a concern for runtime compilations especially in embedded/constraint 
platform.

Based on this, one alternative I would like to propose - how about we use old 
mechanism for standard OpenCL extensions that come from the Spec and dynamic 
maps for vendor extensions? We can then have an early return to avoid the 
expensive checks if  the maps of extensions are empty (OpenCLTypeExtMap, 
OpenCLDeclExtMap) or full check if they contain any elements. In this case if 
vendors are fine with having slight overhead of compilation time they can use 
the feature to add their extensions flexibly, otherwise they will have to stay 
with an old approach of extending Clang directly. What do you think about it? 
Any other proposals are welcome too!


https://reviews.llvm.org/D21698



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

Re: [PATCH] D21698: [OpenCL] Allow disabling types and declarations associated with extensions

[Yaxun Liu via cfe-commits]

yaxunl updated this revision to Diff 70738.
yaxunl marked 5 inline comments as done.
yaxunl added a comment.

Revised by Anastasia's comments.


https://reviews.llvm.org/D21698

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/OpenCLImageTypes.def
  include/clang/Basic/OpenCLOptions.h
  include/clang/Sema/Overload.h
  include/clang/Sema/Sema.h
  include/clang/Serialization/ASTReader.h
  lib/Basic/Targets.cpp
  lib/Frontend/InitPreprocessor.cpp
  lib/Headers/opencl-c.h
  lib/Parse/ParsePragma.cpp
  lib/Parse/Parser.cpp
  lib/Sema/DeclSpec.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/CodeGenOpenCL/extension-begin.cl
  test/Parser/opencl-atomics-cl20.cl
  test/Parser/opencl-pragma.cl
  test/SemaOpenCL/extension-begin.cl

Index: test/SemaOpenCL/extension-begin.cl
===
--- /dev/null
+++ test/SemaOpenCL/extension-begin.cl
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only
+
+#pragma OPENCL EXTENSION all : begin // expected-warning {{expected 'disable' - ignoring}}
+#pragma OPENCL EXTENSION all : end // expected-warning {{expected 'disable' - ignoring}}
+
+#pragma OPENCL EXTENSION my_ext : begin 
+
+struct A {
+  int a;
+};
+
+void f(void);
+
+__attribute__((overloadable)) void g(long x);
+
+#pragma OPENCL EXTENSION my_ext : end
+#pragma OPENCL EXTENSION my_ext : end // expected-warning {{OpenCL extension end directive mismatches begin directive - ignoring}}
+
+__attribute__((overloadable)) void g(void);
+
+#pragma OPENCL EXTENSION my_ext : enable
+void test_f1(void) {
+  struct A test_A1;
+  f();
+  g(0);
+}
+
+#pragma OPENCL EXTENSION my_ext : disable 
+void test_f2(void) {
+  struct A test_A2; // expected-error {{use of type 'struct A' requires my_ext extension to be enabled}}
+  f(); // expected-error {{use of declaration requires my_ext extension to be enabled}}
+  g(0); // expected-error {{no matching function for call to 'g'}}
+// expected-note@-19 {{candidate disabled due to OpenCL extension}}
+// expected-note@-15 {{candidate function not viable: requires 0 arguments, but 1 was provided}}
+}
Index: test/Parser/opencl-pragma.cl
===
--- test/Parser/opencl-pragma.cl
+++ test/Parser/opencl-pragma.cl
@@ -5,9 +5,9 @@
 #pragma OPENCL EXTENSION cl_no_such_extension : disable /* expected-warning {{unknown OpenCL extension 'cl_no_such_extension' - ignoring}} */
 
 #pragma OPENCL EXTENSION all : disable
-#pragma OPENCL EXTENSION all : enable /* expected-warning {{unknown OpenCL extension 'all' - ignoring}} */
+#pragma OPENCL EXTENSION all : enable /* expected-warning {{expected 'disable' - ignoring}} */
 
-#pragma OPENCL EXTENSION cl_khr_fp64 : on /* expected-warning {{expected 'enable' or 'disable' - ignoring}} */
+#pragma OPENCL EXTENSION cl_khr_fp64 : on /* expected-warning {{expected 'enable', 'disable', 'begin' or 'end' - ignoring}} */
 
 #pragma OPENCL FP_CONTRACT ON
 #pragma OPENCL FP_CONTRACT OFF
Index: test/Parser/opencl-atomics-cl20.cl
===
--- test/Parser/opencl-atomics-cl20.cl
+++ test/Parser/opencl-atomics-cl20.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 -fsyntax-only -cl-std=CL2.0 -DCL20
+// RUN: %clang_cc1 %s -triple spir64-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0 -DCL20
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0 -DCL20 -DEXT -Wpedantic-core-features
 
 #ifdef EXT
@@ -47,14 +48,16 @@
 // expected-error@-28 {{use of type 'atomic_ulong' (aka '_Atomic(unsigned long)') requires cl_khr_int64_extended_atomics extension to be enabled}}
 // expected-error@-27 {{use of type 'atomic_double' (aka '_Atomic(double)') requires cl_khr_int64_base_atomics extension to be enabled}}
 // expected-error@-28 {{use of type 'atomic_double' (aka '_Atomic(double)') requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re@-27 {{use of type 'atomic_intptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-28 {{use of type 'atomic_intptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re@-28 {{use of type 'atomic_uintptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-29 {{use of type 'atomic_uintptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re@-29 {{use of type 'atomic_size_t' 

Re: [PATCH] D21698: [OpenCL] Allow disabling types and declarations associated with extensions

[Yaxun Liu via cfe-commits]

yaxunl marked 8 inline comments as done.
yaxunl added a comment.

F2413912: ___10_Fiji.i 

F2413913: callgrind.out.20440 

Uploaded the kernel used for profiling and the calgrind data. This kernel is 
dumped from the open source ray tracer Blender Cycles. It is a large kernel 
with lots of declarations.



Comment at: include/clang/Basic/DiagnosticParseKinds.td:957
@@ +956,3 @@
+  "expected %select{'enable', 'disable', 'begin' or 'end'|'disable'}0 - 
ignoring">, InGroup;
+def warn_pragma_begin_end_mismatch : Warning<
+  "OpenCL extension end directive mismatches begin directive - ignoring">, 
InGroup;

Anastasia wrote:
> Could you please add a test for this diagnostic too?
added


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7998
@@ -7995,1 +7997,3 @@
   InGroup;
+ def err_decl_requires_extension : Error<
+  "use of declaration requires %0 extension to be enabled">;

Anastasia wrote:
> Could we unify with err_type_requires_extension?
these two error msgs have different format.

  def err_type_requires_extension : Error<
   "use of type %0 requires %1 extension to be enabled">;

err_type_requires_extension has an extra type name argument.


Comment at: lib/Headers/opencl-c.h:15484
@@ +15483,3 @@
+#ifdef cl_khr_gl_msaa_sharing
+#pragma OPENCL EXTENSION cl_khr_gl_msaa_sharing : enable
+#endif //cl_khr_gl_msaa_sharing

Anastasia wrote:
> Does this change belong here?
With this feature, use of image types associated with an extension requires 
enabling the extension. That's why this change.


Comment at: lib/Parse/ParsePragma.cpp:461
@@ -459,1 +460,3 @@
+  };
+  typedef std::pair OpenCLExtData;
 }

Anastasia wrote:
> Could we use PointerIntPair still but with 2 bits?
Using PointerIntPair with 2 bits requires IdentifierInfo to be aligned at 4 
bytes, however this is not true. IdentifierInfo has no explicit alignment 
specifier, therefore it can only hold 1 bit in PointerIntPair.


Comment at: lib/Sema/Sema.cpp:1558
@@ +1557,3 @@
+  if (Exts.empty())
+return;
+  auto CanT = T.getCanonicalType().getTypePtr();

Anastasia wrote:
> Would this not be an error?
This function can be called grammatically to set extensions for a group of 
image types, e.g., the extensions associated with image types. It is more 
convenient to allow empty string here. If empty string is not allowed, it can 
be diagnosed before calling this function.


Comment at: lib/Sema/SemaDecl.cpp:4797
@@ +4796,3 @@
+  if (getLangOpts().OpenCL)
+setCurrentOpenCLExtensionForDecl(Dcl);
+

Anastasia wrote:
> I am a  bit concerned about the compilation speed since we will be performing 
> expensive container lookups for each declaration here.
> 
> Have you done any profiling in this direction?
Profiling result shows cost of checking disabled types and declarations can be 
ignored for typical OpenCL kernels.


Comment at: lib/Serialization/ASTReader.cpp:6937
@@ -6936,3 +6942,1 @@
 
-  // FIXME: What happens if these are changed by a module import?
-  if (!OpenCLExtensions.empty()) {

Anastasia wrote:
> Is the comment no longer relevant?
This change will initialize Sema with OpenCLExtensions imported from 
module/pch, so the original comment no longer applies.


https://reviews.llvm.org/D21698



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

Re: [PATCH] D21698: [OpenCL] Allow disabling types and declarations associated with extensions

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D21698#537156, @yaxunl wrote:

> I did profiling with valgrind for the cost of checking disabled types and 
> declarations. For a typical program, time spent in checking disabled types 
> and declarations are less than 0.1%, so this cost can be ignored.


Could you please provide the profiled code example here please? As far as I 
understand this lookup will run for all the declarations and function calls. So 
we should make sure to profile various programs to see what the impact is with 
different use cases.


https://reviews.llvm.org/D21698



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

Re: [PATCH] D21698: [OpenCL] Allow disabling types and declarations associated with extensions

[Yaxun Liu via cfe-commits]

yaxunl added a comment.

I did profiling with valgrind for the cost of checking disabled types and 
declarations. For a typical program, time spent in checking disabled types and 
declarations are less than 0.1%, so this cost can be ignored.


https://reviews.llvm.org/D21698



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

Re: [PATCH] D21698: [OpenCL] Allow disabling types and declarations associated with extensions

[Anastasia Stulova via cfe-commits]

Anastasia requested changes to this revision.
Anastasia added a comment.
This revision now requires changes to proceed.

Have you done any investigation regarding the compilation speed as this change 
adds expensive container lookups for all OpenCL declarations and function calls.

It would be nice to see some profiling information first before we go ahead 
with this solution...



Comment at: include/clang/Basic/DiagnosticParseKinds.td:957
@@ +956,3 @@
+  "expected %select{'enable', 'disable', 'begin' or 'end'|'disable'}0 - 
ignoring">, InGroup;
+def warn_pragma_begin_end_mismatch : Warning<
+  "OpenCL extension end directive mismatches begin directive - ignoring">, 
InGroup;

Could you please add a test for this diagnostic too?


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7998
@@ -7995,1 +7997,3 @@
   InGroup;
+ def err_decl_requires_extension : Error<
+  "use of declaration requires %0 extension to be enabled">;

Could we unify with err_type_requires_extension?


Comment at: include/clang/Basic/OpenCLImageTypes.def:20
@@ -17,4 +19,3 @@
 
-#define IMAGE_READ_TYPE(Type, Id) GENERIC_IMAGE_TYPE(Type, Id)
-#define IMAGE_WRITE_TYPE(Type, Id) 
-#define IMAGE_READ_WRITE_TYPE(Type, Id) 
+#define IMAGE_READ_TYPE(Type, Id, ...) GENERIC_IMAGE_TYPE(Type, Id)
+#define IMAGE_WRITE_TYPE(Type, Id, ...)

Any reason to use variadic macros here? Do we expect variable number of 
arguments?


Comment at: include/clang/Basic/OpenCLOptions.h:18
@@ -17,1 +17,3 @@
 
+#include 
+#include 

Are those two headers actually used here?


Comment at: include/clang/Sema/Sema.h:8006
@@ +8005,3 @@
+  /// \brief Check if declaration \p D used by expression \p E
+  /// is disabled due to required OpenCL extensions are disabled. If so,
+  /// emit diagnostics.

are disabled -> being disabled


Comment at: lib/Headers/opencl-c.h:15484
@@ +15483,3 @@
+#ifdef cl_khr_gl_msaa_sharing
+#pragma OPENCL EXTENSION cl_khr_gl_msaa_sharing : enable
+#endif //cl_khr_gl_msaa_sharing

Does this change belong here?


Comment at: lib/Parse/ParsePragma.cpp:461
@@ -459,1 +460,3 @@
+  };
+  typedef std::pair OpenCLExtData;
 }

Could we use PointerIntPair still but with 2 bits?


Comment at: lib/Sema/Sema.cpp:1558
@@ +1557,3 @@
+  if (Exts.empty())
+return;
+  auto CanT = T.getCanonicalType().getTypePtr();

Would this not be an error?


Comment at: lib/Sema/SemaDecl.cpp:4797
@@ +4796,3 @@
+  if (getLangOpts().OpenCL)
+setCurrentOpenCLExtensionForDecl(Dcl);
+

I am a  bit concerned about the compilation speed since we will be performing 
expensive container lookups for each declaration here.

Have you done any profiling in this direction?


Comment at: lib/Serialization/ASTReader.cpp:6937
@@ -6936,3 +6942,1 @@
 
-  // FIXME: What happens if these are changed by a module import?
-  if (!OpenCLExtensions.empty()) {

Is the comment no longer relevant?


https://reviews.llvm.org/D21698



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