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

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.
Herald added a subscriber: Naghasan.

> 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.

That's sounds good to me. Especially because this states nothing about if block has captures/no captures. Originally, I was thinking about something like: //blocks in constant address space require program scope variables feature support//, but that's too much relies on concrete ABI.



================
Comment at: clang/lib/AST/ExprConstant.cpp:9554
+
+    const auto &OpenCLFeaturesMap =
+        Info.Ctx.getTargetInfo().getSupportedOpenCLOpts();
----------------
Anastasia wrote:
> What test case covers this change? It feels like something we should try to diagnose earlier...
The test case which exactly was added. Since blocks in constant address space are disallowed at this point, we can treat all other blocks with no captures not as constant expressions - it will make CodeGen generate block  literal in local scope for blocks with no captures. See `buildGlobalBlock `and `CodeGenModule::GetAddrOfGlobalBlock` for details.


================
Comment at: clang/lib/CodeGen/CGOpenCLRuntime.cpp:191
+// feature support.
+bool CGOpenCLRuntime::blockCanBeGlobal() {
+  const auto &LO = CGM.getLangOpts();
----------------
Anastasia wrote:
> This function feels like something that belongs better to `OpenCLOptions` rather than `CodeGen`?
IIRC we can't query Sema from a CodeGen... The alternative would be to introduce a new language option, which is not desirable.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:8024
     if (T->isBlockPointerType()) {
+      if ((T.getAddressSpace() == LangAS::opencl_constant) &&
+          !getOpenCLOptions().areProgramScopeVariablesSupported(
----------------
Anastasia wrote:
> 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...
Well, unfortunately there are already few cases in CTS coverage which rely on that... Note that this is an address space of a block, but not a block literal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112230