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

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

Reply via email to