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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
