[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<IgnoredPragmas>;
+def warn_pragma_begin_end_mismatch : Warning<
+  "OpenCL extension end directive mismatches begin directive - ignoring">, InGroup<IgnoredPragmas>;
----------------
Could you please add a test for this diagnostic too?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7998
@@ -7995,1 +7997,3 @@
   InGroup<IgnoredAttributes>;
+ 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 <string>
+#include <vector>
----------------
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<const IdentifierInfo *, OpenCLExtState> 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