[PATCH] D112230: [OpenCL] Add support of __opencl_c_device_enqueue feature macro.

Mon Nov 8 04:57:08 PST 2021

Anastasia added a comment.

In D112230#3078231 <https://reviews.llvm.org/D112230#3078231>, @azabaznov wrote:

> @Anastasia, @yaxunl, do you think it's possible to refactor code generation for blocks such that block literal for global blocks (with no captures) would be emitted in constant address space? Now it's emitted in global address space (for example @__block_literal_global in https://godbolt.org/z/4z8hGj7hz).

I think that the main intent of using `generic` was to simplify the code generation to the block invoke function, as it could always just use generic address space regardless the block kind, see `@my_block_A_block_invoke` in https://godbolt.org/z/956E1KrPP.

However it is probably possible to refactor clang to emit exact address space depending on the type of the block literal. However, it would break ABI for OpenCL 2.0 which is undesirable and also it would add extra complexity in already undocumented and hard to understand code. Plus there are always uncertainties around the exact implementation of blocks. So we would need a pretty decent prototype before we confirm this is actually even doable. Unless we find a strong need for this I would prefer to avoid this refactoring.

How about we clarify with Khronos whether it would be sufficient to add a restriction like:

> Program scope blocks are only supported when program scope variables feature is supported.

And then see if this is acceptable route forward?

================
Comment at: clang/lib/AST/ExprConstant.cpp:9554
+
+    const auto &OpenCLFeaturesMap =
+        Info.Ctx.getTargetInfo().getSupportedOpenCLOpts();
----------------
What test case covers this change? It feels like something we should try to diagnose earlier...

================
Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:191
+// feature support.
+bool CGOpenCLRuntime::blockCanBeGlobal() {
+  const auto &LO = CGM.getLangOpts();
----------------
This function feels like something that belongs better to `OpenCLOptions` rather than `CodeGen`?

================
Comment at: clang/lib/Sema/SemaDecl.cpp:8024
     if (T->isBlockPointerType()) {
+      if ((T.getAddressSpace() == LangAS::opencl_constant) &&
+          !getOpenCLOptions().areProgramScopeVariablesSupported(
----------------
Good spot, I didn't feel the intent was to allow qualifying the block by an address space... but I don't think this was ever clarified. I feel that the assumption was that blocks would always be in global address space...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112230