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