[PATCH] D38134: [OpenCL] Emit enqueued block as kernel

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 3 inline comments as done.
yaxunl added a comment.

In https://reviews.llvm.org/D38134#877831, @Anastasia wrote:

> Now if we have a block which is being called and enqueued at the same time, will we generate 2 functions for it? Could we add such test case btw?


Yes. It is covered by test/CodeGenOpenCL/cl20-device-side-enqueue.cl, line 246, 250, and 256.

> I feel it would be much simpler if we could always generate the kernel metadata for blocks. A lot of special case code would be removed if we do this. OpenCL doesn't prevent kernel functions to be used just as normal functions (6.7.1) so it should be a perfectly valid thing to do. Do you seen any issues with that?

The special cases in metadata generation code is due to the first argument of LLVM block invoke function is not defined in BlockDecl. Emitting metadata for all block invoke functions does not help here.



================
Comment at: lib/CodeGen/CGBlocks.cpp:1255
     // Allocate a stack slot to let the debug info survive the RA.
-    Address alloc = CreateMemTemp(D->getType(), D->getName() + ".addr");
+    Address alloc = CreateMemTemp(
+        !PV.isIndirect() ? D->getType()
----------------
Anastasia wrote:
> Is there any test that covers this?
Yes. This is covered by test/CodeGenOpenCL/amdgpu-enqueue-kernel.cl where the block struct is passed directly to the kernel instead of by a pointer.


================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:113
+
+llvm::Value *CGOpenCLRuntime::emitOpenCLEnqueuedBlock(CodeGenFunction &CGF,
+                                                      const Expr *E) {
----------------
Anastasia wrote:
> I am not particularly in favour of duplicating CodeGen functionality as it typically has so many special cases that are hard to catch. Is this logic needed in order to pass to block literal information  that the block is enqueued?
This code is needed to emit separate functions for a block which is directly called and also enqueued as a kernel. Since the kernel needs to have proper calling convention and ABI, it cannot be emitted as the same function as when the block is called directly. Since it is OpenCL specific code, I found it is cleaner to separate this code as member of CGOpenCLRuntime instead of fitting it into CGF.EmitBlockLiteral.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:535
+    if (i == 0 && IsBlock) {
+      ty = CGF.CGM.getTargetCodeGenInfo().getEnqueuedBlockArgumentType(
+          ASTCtx, *CGF.BlockInfo);
----------------
Anastasia wrote:
> I don't quite understand why we need to special case this? As far as I undertsnad block argument is a `generic void* ` type but it's being cast to a concrete block struct inside the block function. Do we gain anything from having it a specific type here?
This argument is not part of BlockDecl. BlockDecl only has arguments shown up in the source code. The first argument in the LLVM block invoke function is generated by codegen and there is no correspondence in AST, so it has to be handled as a special case.


https://reviews.llvm.org/D38134