[PATCH] D120265: AMDGPU: Use the implicit kernargs for code object version 5

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16255
+      IsCOV_5 ? EmitAMDGPUImplicitArgPtr(CGF) : EmitAMDGPUDispatchPtr(CGF);
   // Indexing the HSA kernel_dispatch_packet struct.
   auto *Offset = llvm::ConstantInt::get(CGF.Int32Ty, XOffset + Index * 2);
----------------
I agree with Matt above; this comment seems like it should also be updated for the v5 case?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:4388
     return ArgOffset + 4;
+  // TODO: if the offset changes with code pbject version, we should include
+  // code object version in the calculation.
----------------
typo


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:1817
+    AMDGPUTargetLowering::ImplicitParameter Param =
+        (AS == AMDGPUAS::LOCAL_ADDRESS) ? AMDGPUTargetLowering::SHARED_BASE
+                                        : AMDGPUTargetLowering::PRIVATE_BASE;
----------------
These parens are redundant


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:155
 
+unsigned getAmdhsaCodeObjectVersion() { return AmdhsaCodeObjectVersion; }
+
----------------
cfang wrote:
> arsenm wrote:
> > The code object version should probably come from the IR, not a global opt
> Can you be explicit how to get code object version from the IR? 
IIUC the global opt var is the best we have right now, and any improvement to that situation is orthogonal to this change. I would vote that this not block the patch under review


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

https://reviews.llvm.org/D120265