[PATCH] D38610: [AMDGPU] Lower enqueued blocks and generate runtime metadata

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUOpenCLEnqueuedBlockLowering.cpp:76-78
+      if (!I.hasOneUse() || !I.user_begin()->hasOneUse() ||
+          !isa<ConstantExpr>(*I.user_begin()) ||
+          !isa<ConstantExpr>(*I.user_begin()->user_begin())) {
----------------
arsenm wrote:
> Looking at the users like this confuse me. Don't all uses need to be handled always? Why isn't this just a range loop over all the users?
We only translate IRs emitted by Clang when translating enqueue_kernel functions. These IRs satisfy the conditions given here. Therefore we only need to handle the first use.

If IR does not satisfy these conditions, it is not generated by Clang and could be anything. We just do not process them. 


================
Comment at: lib/Target/AMDGPU/AMDGPUOpenCLEnqueuedBlockLowering.cpp:78
+          !isa<ConstantExpr>(*I.user_begin()) ||
+          !isa<ConstantExpr>(*I.user_begin()->user_begin())) {
+        continue;
----------------
rampitec wrote:
> Users of user_begin() can be empty.
we have check `I.user_begin()->hasOneUse()`.


================
Comment at: lib/Target/AMDGPU/AMDGPUOpenCLEnqueuedBlockLowering.cpp:86
+          M, Type::getInt8Ty(C)->getPointerTo(AS.GLOBAL_ADDRESS),
+          /*IsConstant=*/true, GlobalValue::ExternalLinkage,
+          /*Initializer=*/nullptr, RuntimeHandle, /*InsertBefore=*/nullptr,
----------------
arsenm wrote:
> Why external linkage?
This needs to be an external symbol in code object. Runtime will allocate a global buffer for it and save the kernel address to it.


https://reviews.llvm.org/D38610