[PATCH] D141700: AMDGPU: Move enqueued block handling into clang

[Sameer Sahasrabuddhe via Phabricator via cfe-commits]

sameerds added a comment.

LGTM, to the extent that I can see that the change does what is advertised, and the ultimately emitted HSA metadata preserves the current contract with the runtime.

A couple of tests can use a little more explanatory comments as noted.



================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:12581
+      Mod, HandleTy,
+      /*isConstant=*/true, llvm::GlobalValue::InternalLinkage,
+      /*Initializer=*/RuntimeHandleInitializer, RuntimeHandleName,
----------------
jmmartinez wrote:
> Just a cosmetical remark: Is there any reason to keep the `/*isConstant=*/`, `/*Initializer=*/`, ... comments? I think it would be better to avoid them.
FWIW, I find these comments very helpful when spelunking through code. I could sympathise with not needing `Initializer=` because the value name makes it clear. But an undecorated constant literal like "true" or "10" or "nullptr" works best when accompanied by a comment.


================
Comment at: llvm/test/Bitcode/amdgpu-autoupgrade-enqueued-block.ll:69
+
+; __enqueue_kernel* functions may get inlined
+define amdgpu_kernel void @inlined_caller(ptr addrspace(1) %a, i8 %b, ptr addrspace(1) %c, i64 %d) {
----------------
I did not understand what is being tested here.


================
Comment at: llvm/test/CodeGen/AMDGPU/amdgpu-export-kernel-runtime-handles.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --check-attributes --check-globals
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-export-kernel-runtime-handles < %s | FileCheck %s
+
----------------
Is there any visible effect of the pass being tested? Or the intention is simply to check that the output is the same as input, and there is no error?


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

https://reviews.llvm.org/D141700