[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 <https://reviews.llvm.org/F2413912>

F2413913: callgrind.out.20440 <https://reviews.llvm.org/F2413913>

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<IgnoredPragmas>;
+def warn_pragma_begin_end_mismatch : Warning<
+  "OpenCL extension end directive mismatches begin directive - ignoring">, InGroup<IgnoredPragmas>;
----------------
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<IgnoredAttributes>;
+ 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<const IdentifierInfo *, OpenCLExtState> 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