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

[Yaxun Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
yaxunl marked 2 inline comments as done.
Closed by commit rL289979: [OpenCL] Allow disabling types and declarations 
associated with extensions (authored by yaxunl).

Changed prior to commit:
  https://reviews.llvm.org/D21698?vs=81471=81782#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D21698

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

Index: cfe/trunk/include/clang/Serialization/ASTReader.h
===
--- cfe/trunk/include/clang/Serialization/ASTReader.h
+++ cfe/trunk/include/clang/Serialization/ASTReader.h
@@ -807,7 +807,13 @@
   SourceLocation PointersToMembersPragmaLocation;
 
   /// \brief The OpenCL extension settings.
-  SmallVector OpenCLExtensions;
+  OpenCLOptions OpenCLExtensions;
+
+  /// \brief Extensions required by an OpenCL type.
+  llvm::DenseMap> OpenCLTypeExtMap;
+
+  /// \brief Extensions required by an OpenCL declaration.
+  llvm::DenseMap> OpenCLDeclExtMap;
 
   /// \brief A list of the namespaces we've seen.
   SmallVector KnownNamespaces;
Index: cfe/trunk/include/clang/Serialization/ASTWriter.h
===
--- cfe/trunk/include/clang/Serialization/ASTWriter.h
+++ cfe/trunk/include/clang/Serialization/ASTWriter.h
@@ -459,6 +459,8 @@
   void WriteDeclContextVisibleUpdate(const DeclContext *DC);
   void WriteFPPragmaOptions(const FPOptions );
   void WriteOpenCLExtensions(Sema );
+  void WriteOpenCLExtensionTypes(Sema );
+  void WriteOpenCLExtensionDecls(Sema );
   void WriteCUDAPragmas(Sema );
   void WriteObjCCategories();
   void WriteLateParsedTemplates(Sema );
Index: cfe/trunk/include/clang/Serialization/ASTBitCodes.h
===
--- cfe/trunk/include/clang/Serialization/ASTBitCodes.h
+++ cfe/trunk/include/clang/Serialization/ASTBitCodes.h
@@ -344,7 +344,7 @@
   ///
   /// The TYPE_OFFSET constant describes the record that occurs
   /// within the AST block. The record itself is an array of offsets that
-  /// point into the declarations and types block (identified by 
+  /// point into the declarations and types block (identified by
   /// DECLTYPES_BLOCK_ID). The index into the array is based on the ID
   /// of a type. For a given type ID @c T, the lower three bits of
   /// @c T are its qualifiers (const, volatile, restrict), as in
@@ -446,10 +446,10 @@
 
   /// \brief Record code for the set of ext_vector type names.
   EXT_VECTOR_DECLS = 16,
-  
+
   /// \brief Record code for the array of unused file scoped decls.
   UNUSED_FILESCOPED_DECLS = 17,
-  
+
   /// \brief Record code for the table of offsets to entries in the
   /// preprocessing record.
   PPD_ENTITIES_OFFSETS = 18,
@@ -465,7 +465,7 @@
   /// \brief Record code for an update to the TU's lexically contained
   /// declarations.
   TU_UPDATE_LEXICAL = 22,
-  
+
   // ID 23 used to be for a list of local redeclarations.
 
   /// \brief Record code for declarations that Sema keeps references of.
@@ -490,15 +490,15 @@
 
   // ID 30 used to be a decl update record. These are now in the DECLTYPES
   // block.
-  
+
   // ID 31 used to be a list of offsets to DECL_CXX_BASE_SPECIFIERS records.
 
   /// \brief Record code for \#pragma diagnostic mappings.
   DIAG_PRAGMA_MAPPINGS = 32,
 
   /// \brief Record code for special CUDA declarations.
   CUDA_SPECIAL_DECL_REFS = 33,
-  
+
   /// \brief Record code for header search information.
   HEADER_SEARCH_TABLE = 34,
 
@@ -516,29 +516,29 @@
   KNOWN_NAMESPACES = 38,
 
   /// \brief 

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

[Yaxun Liu via Phabricator via cfe-commits]

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

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

> LGTM! Small nitpicks below can be done before committing. Also it would be 
> nice to double check the compile time is still fine after the last rebase.


Thanks. I did the measurement again and did not see significant difference in 
compilation time before/after this change.




Comment at: include/clang/Basic/OpenCLOptions.h:43
 
-  // Enable or disable all options.
-  void setAll(bool Enable = true) {
-#define OPENCLEXT(nm)   nm = Enable;
-#include "clang/Basic/OpenCLExtensions.def"
+  // Is supported OpenCL extension or (optional) core feature for OpenCL 
version
+  // \p CLVer.

Anastasia wrote:
> Did you mean "and (optional) core feature?"
My understanding is that once an extension becomes an (optional) core feature, 
it is no longer called an extension. So for for a specific OpenCL version, \p 
Ext is either is an extension, or an (optional) core feature.  However this 
wording may be confusing. How about rewording as

Is supported as either an extension or an (optional) core feature for OpenCL 
version \p CLVer



Comment at: test/SemaOpenCL/extension-begin.cl:5
+// Test with pch.
+// RUN: %clang_cc1 %s -DHEADER -triple spir-unknown-unknown -emit-pch 
-DHEADER_ONLY -o %t -verify -pedantic
+// RUN: %clang_cc1 %s -DHEADER_USER -triple spir-unknown-unknown -include-pch 
%t -fsyntax-only -verify -pedantic

Anastasia wrote:
> Do we need -DHEADER_ONLY here?
I will remove it.


https://reviews.llvm.org/D21698



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

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

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Small nitpicks below can be done before committing. Also it would be nice 
to double check the compile time is still fine after the last rebase.

Thanks!




Comment at: include/clang/Basic/OpenCLOptions.h:28
+unsigned Avail; // Option starts to be available in this OpenCL version
+unsigned Core;  // Options becomes (optional) core feature in this OpenCL
+// version

Options -> Option



Comment at: include/clang/Basic/OpenCLOptions.h:43
 
-  // Enable or disable all options.
-  void setAll(bool Enable = true) {
-#define OPENCLEXT(nm)   nm = Enable;
-#include "clang/Basic/OpenCLExtensions.def"
+  // Is supported OpenCL extension or (optional) core feature for OpenCL 
version
+  // \p CLVer.

Did you mean "and (optional) core feature?"



Comment at: test/SemaOpenCL/extension-begin.cl:5
+// Test with pch.
+// RUN: %clang_cc1 %s -DHEADER -triple spir-unknown-unknown -emit-pch 
-DHEADER_ONLY -o %t -verify -pedantic
+// RUN: %clang_cc1 %s -DHEADER_USER -triple spir-unknown-unknown -include-pch 
%t -fsyntax-only -verify -pedantic

Do we need -DHEADER_ONLY here?


https://reviews.llvm.org/D21698



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

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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 81471.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.

Added serialisation of types and declarations associated with extensions.
Brought the patch up to date.


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/Basic/TargetInfo.h
  include/clang/Sema/Overload.h
  include/clang/Sema/Sema.h
  include/clang/Serialization/ASTBitCodes.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ASTWriter.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
  test/SemaOpenCL/extensions.cl

Index: test/SemaOpenCL/extensions.cl
===
--- test/SemaOpenCL/extensions.cl
+++ test/SemaOpenCL/extensions.cl
@@ -22,8 +22,6 @@
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64 -cl-ext=+cl_khr_fp16 -cl-ext=-cl_khr_fp64 -DNOFP64
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64,-cl_khr_fp64,+cl_khr_fp16 -DNOFP64
 
-
-
 #ifdef FP64
 // expected-no-diagnostics
 #endif
Index: test/SemaOpenCL/extension-begin.cl
===
--- /dev/null
+++ test/SemaOpenCL/extension-begin.cl
@@ -0,0 +1,54 @@
+// Test this without pch.
+// RUN: %clang_cc1 %s -DHEADER -DHEADER_USER -triple spir-unknown-unknown -verify -pedantic -fsyntax-only
+
+// Test with pch.
+// RUN: %clang_cc1 %s -DHEADER -triple spir-unknown-unknown -emit-pch -DHEADER_ONLY -o %t -verify -pedantic
+// RUN: %clang_cc1 %s -DHEADER_USER -triple spir-unknown-unknown -include-pch %t -fsyntax-only -verify -pedantic
+
+#ifdef HEADER
+
+#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;
+};
+
+typedef struct A TypedefOfA;
+typedef const TypedefOfA* PointerOfA;
+
+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);
+
+#endif // HEADER
+
+#ifdef HEADER_USER
+
+#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}}
+  const struct A test_A_local; // expected-error {{use of type 'struct A' requires my_ext extension to be enabled}}
+  TypedefOfA test_typedef_A; // expected-error {{use of type 'TypedefOfA' (aka 'struct A') requires my_ext extension to be enabled}}
+  PointerOfA test_A_pointer; // expected-error {{use of type 'PointerOfA' (aka 'const 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@-26 {{candidate disabled due to OpenCL extension}}
+// expected-note@-22 {{candidate function not viable: requires 0 arguments, but 1 was provided}}
+}
+
+#endif // HEADER_USER
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

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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 6 inline comments as done.
yaxunl added inline comments.



Comment at: lib/Serialization/ASTReader.cpp:3167
 case OPENCL_EXTENSIONS:
-  // Later tables overwrite earlier ones.
-  OpenCLExtensions.swap(Record);

Anastasia wrote:
> Btw, OpenCLTypeExtMap and OpenCLTypeDeclMap don't have to be serialized?
Yes. I will add it.



Comment at: test/Parser/opencl-atomics-cl20.cl:51
 // 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' (aka '_Atomic({{.+}})') 
requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-30 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})') 
requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re@-30 {{use of type 'atomic_ptrdiff_t' (aka 
'_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-31 {{use of type 'atomic_ptrdiff_t' (aka 
'_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be 
enabled}}
+#if __LP64__
+// expected-error-re@-28 {{use of type 'atomic_intptr_t' (aka 
'_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}

Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > Why this change?
> > atomic_intptr_t etc. requires cl_khr_int64_extended_atomics only on 64 bit 
> > platforms.
> > 
> > This is a bug which was fixed by this patch.
> The spec says:
> "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."
> 
> This seems to be the same as long and double?
Yes. Use of long and double were correctly diagnosed if the corresponding 
extension was disabled but size_t/intptr_t/uintptr_t/ptrdiff_t was not. This 
patch fixes that.


https://reviews.llvm.org/D21698



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

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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/Sema/Sema.h:8081
+  /// \brief Set current OpenCL extensions for a type which can only be used
+  /// when these OpenCL extensions are enabled. If current OpenCL Extsion is
+  /// empty, do nothing.

Extsion -> extension



Comment at: include/clang/Sema/Sema.h:8120
 
+  /// Checks if a type or declaration is disabled due to the owning extension
+  /// is disabled, and emits diagnostic messages if it is disabled.

or declaration -> or a declaration



Comment at: include/clang/Sema/Sema.h:8121
+  /// Checks if a type or declaration is disabled due to the owning extension
+  /// is disabled, and emits diagnostic messages if it is disabled.
+  /// \param D type or declaration to be checked.

is disabled -> being disabled



Comment at: lib/Serialization/ASTReader.cpp:3167
 case OPENCL_EXTENSIONS:
-  // Later tables overwrite earlier ones.
-  OpenCLExtensions.swap(Record);

Btw, OpenCLTypeExtMap and OpenCLTypeDeclMap don't have to be serialized?



Comment at: test/Parser/opencl-atomics-cl20.cl:51
 // 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' (aka '_Atomic({{.+}})') 
requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-30 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})') 
requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re@-30 {{use of type 'atomic_ptrdiff_t' (aka 
'_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-31 {{use of type 'atomic_ptrdiff_t' (aka 
'_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be 
enabled}}
+#if __LP64__
+// expected-error-re@-28 {{use of type 'atomic_intptr_t' (aka 
'_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}

yaxunl wrote:
> Anastasia wrote:
> > Why this change?
> atomic_intptr_t etc. requires cl_khr_int64_extended_atomics only on 64 bit 
> platforms.
> 
> This is a bug which was fixed by this patch.
The spec says:
"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."

This seems to be the same as long and double?


https://reviews.llvm.org/D21698



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

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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 81142.
yaxunl marked 9 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/Basic/TargetInfo.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
  test/SemaOpenCL/extensions.cl

Index: test/SemaOpenCL/extensions.cl
===
--- test/SemaOpenCL/extensions.cl
+++ test/SemaOpenCL/extensions.cl
@@ -21,8 +21,6 @@
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64 -cl-ext=+cl_khr_fp16 -cl-ext=-cl_khr_fp64 -DNOFP64
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64,-cl_khr_fp64,+cl_khr_fp16 -DNOFP64
 
-
-
 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}}
Index: test/SemaOpenCL/extension-begin.cl
===
--- /dev/null
+++ test/SemaOpenCL/extension-begin.cl
@@ -0,0 +1,41 @@
+// 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;
+};
+
+typedef struct A TypedefOfA;
+typedef const TypedefOfA* PointerOfA;
+
+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}}
+  const struct A test_A_local; // expected-error {{use of type 'struct A' requires my_ext extension to be enabled}}
+  TypedefOfA test_typedef_A; // expected-error {{use of type 'TypedefOfA' (aka 'struct A') requires my_ext extension to be enabled}}
+  PointerOfA test_A_pointer; // expected-error {{use of type 'PointerOfA' (aka 'const 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@-22 {{candidate disabled due to OpenCL extension}}
+// expected-note@-18 {{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 

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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8127
   "invalid prototype, variadic arguments are not allowed in OpenCL">;
+def err_requires_extension : Error<
+  "use of %select{declaration|type }0%1 requires %2 extension to be enabled">;

Could we rename err_requires_extension -> err_opencl_requires_extension



Comment at: lib/Parse/ParsePragma.cpp:461
+  };
+  typedef std::pair OpenCLExtData;
 }

yaxunl wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > 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.
> > Based on its structure with having a pointer member I think it's reasonable 
> > to assume at least 4 bytes alignment... Although this doesn't seem to 
> > affect the performance anyways.
> You are right. The alignment of IdentifierInfo should be 8 on most systems.
> 
> However in IdentifierTable.h there is
> 
> 
> ```
> // Provide PointerLikeTypeTraits for IdentifierInfo pointers, which
> // are not guaranteed to be 8-byte aligned.
> template<>
> class PointerLikeTypeTraits {
> public:
>   static inline void *getAsVoidPointer(clang::IdentifierInfo* P) {
> return P;
>   }
>   static inline clang::IdentifierInfo *getFromVoidPointer(void *P) {
> return static_cast(P);
>   }
>   enum { NumLowBitsAvailable = 1 };
> };
> 
> ```
> Due to the above code there is only 1 bit available for when using 
> PointerIntPair with IdentifierInfo*. My guess is that alignment of 
> IdentifierInfo is not 8 on certain systems so some one intentionally defined 
> the above code to prevent using PointerIntPair with IdentifierInfo* for more 
> than 1 bit.
> 
Not too critical, but it's certainly not 8 byte aligned for 32 bit 
architectures. It should although be at least 4 bytes aligned in my opinion.

This code seems to be old, and the comment is not very precise to be honest. 



Comment at: lib/Sema/Sema.cpp:1614
+Decl = TagT->getDecl();
+  auto DL = OpenCLDeclExtMap.find(Decl);
+  if (DL != OpenCLDeclExtMap.end()) {

this bit (up to line 1623) seems to be repeated in multiple places... wondering 
if we could use a helper function instead perhaps with the templates?



Comment at: lib/Serialization/ASTReader.cpp:3170
+OpenCLExtensions.OptMap[Name] = {
+  static_cast(Record[I++]),
+  static_cast(Record[I++]),

Can you put a comment explaining which element each line corresponds to?


https://reviews.llvm.org/D21698



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

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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 80422.
yaxunl marked 6 inline comments as done.
yaxunl added a comment.

Bring the patch up to date.
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/Basic/TargetInfo.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
  test/SemaOpenCL/extensions.cl

Index: test/SemaOpenCL/extensions.cl
===
--- test/SemaOpenCL/extensions.cl
+++ test/SemaOpenCL/extensions.cl
@@ -21,8 +21,6 @@
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64 -cl-ext=+cl_khr_fp16 -cl-ext=-cl_khr_fp64 -DNOFP64
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64,-cl_khr_fp64,+cl_khr_fp16 -DNOFP64
 
-
-
 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}}
Index: test/SemaOpenCL/extension-begin.cl
===
--- /dev/null
+++ test/SemaOpenCL/extension-begin.cl
@@ -0,0 +1,41 @@
+// 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;
+};
+
+typedef struct A TypedefOfA;
+typedef const TypedefOfA* PointerOfA;
+
+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}}
+  const struct A test_A_local; // expected-error {{use of type 'struct A' requires my_ext extension to be enabled}}
+  TypedefOfA test_typedef_A; // expected-error {{use of type 'TypedefOfA' (aka 'struct A') requires my_ext extension to be enabled}}
+  PointerOfA test_A_pointer; // expected-error {{use of type 'PointerOfA' (aka 'const 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@-22 {{candidate disabled due to OpenCL extension}}
+// expected-note@-18 {{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 

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

[Yaxun Liu via Phabricator via cfe-commits]

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

Sorry for the delay. I will update this patch.




Comment at: lib/Parse/ParsePragma.cpp:461
+  };
+  typedef std::pair OpenCLExtData;
 }

Anastasia wrote:
> yaxunl wrote:
> > 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.
> Based on its structure with having a pointer member I think it's reasonable 
> to assume at least 4 bytes alignment... Although this doesn't seem to affect 
> the performance anyways.
You are right. The alignment of IdentifierInfo should be 8 on most systems.

However in IdentifierTable.h there is


```
// Provide PointerLikeTypeTraits for IdentifierInfo pointers, which
// are not guaranteed to be 8-byte aligned.
template<>
class PointerLikeTypeTraits {
public:
  static inline void *getAsVoidPointer(clang::IdentifierInfo* P) {
return P;
  }
  static inline clang::IdentifierInfo *getFromVoidPointer(void *P) {
return static_cast(P);
  }
  enum { NumLowBitsAvailable = 1 };
};

```
Due to the above code there is only 1 bit available for when using 
PointerIntPair with IdentifierInfo*. My guess is that alignment of 
IdentifierInfo is not 8 on certain systems so some one intentionally defined 
the above code to prevent using PointerIntPair with IdentifierInfo* for more 
than 1 bit.




Comment at: lib/Parse/ParsePragma.cpp:1429
   PP.Lex(Tok);
-  if (Tok.isNot(tok::identifier)) {
-PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_enable_disable);
+  if (Tok.isNot(tok::identifier) && Tok.isNot(tok::kw_register)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_predicate) << 0;

Anastasia wrote:
> Not clear why register keyword is here too?
this is old code when I use 'register' to register an extension. will remove it.



Comment at: lib/Sema/Sema.cpp:226
  Context.getAtomicType(Context.UnsignedIntTy));
-  addImplicitTypedef("atomic_long", Context.getAtomicType(Context.LongTy));
-  addImplicitTypedef("atomic_ulong",
- Context.getAtomicType(Context.UnsignedLongTy));
+  auto AtomicLongT = Context.getAtomicType(Context.LongTy);
+  addImplicitTypedef("atomic_long", AtomicLongT);

Anastasia wrote:
> Any reason for changing this too?
To associate atomic long and double types with corresponding extensions so that 
when the extension is disabled, the associated types are also disabled.



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

Anastasia wrote:
> Could we then check for an empty string ExtStr as a first function statement 
> instead?
will do



Comment at: test/Parser/opencl-atomics-cl20.cl:51
 // 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' (aka '_Atomic({{.+}})') 
requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-30 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})') 
requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re@-30 {{use of type 'atomic_ptrdiff_t' (aka 
'_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-31 {{use of type 'atomic_ptrdiff_t' (aka 
'_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be 
enabled}}
+#if __LP64__
+// expected-error-re@-28 {{use of type 'atomic_intptr_t' (aka 
'_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}

Anastasia wrote:
> Why this change?
atomic_intptr_t etc. requires cl_khr_int64_extended_atomics only on 64 bit 
platforms.

This is a bug which was fixed by this patch.



Comment at: test/SemaOpenCL/extension-begin.cl:27
+}
+
+#pragma OPENCL EXTENSION my_ext : disable 

Anastasia wrote:
> Could you please add case with typedef of a type from an 

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