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

[Changpeng Fang via Phabricator via llvm-commits]

cfang marked 2 inline comments as done.
cfang added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:546
+    AAPointerInfo::OffsetAndSize OAS(
+        AMDGPU::ImplicitArg::HEAP_PTR_OFFSET_COV5, 8);
     return funcRetrievesImplicitKernelArg(A, OAS);
----------------
sameerds wrote:
> I think we should keep using the original getHeapPtrImplicitArgPosition(). Hardcoding the enum here doesn't necessarily make the code more readable. And later if we have a different value in COV6, we will end up reintroducing a check for the code-object-version anyway. That check can be encapsulated within the get...ArgPosition() family of functions.
I personally do not have any preference here to use the offset enum or a function. Similarly I also could not understand why the offset of an existing argument changes its value across code object versions. However, I do think this should not block the current work of code object 5.  


================
Comment at: llvm/lib/Target/AMDGPU/SIDefines.h:786
+enum Offset : unsigned {
+  HOSTCALL_PTR_OFFSET_PRIOR_COV5 = 24,
+  HOSTCALL_PTR_OFFSET_COV5 = 80,
----------------
sameerds wrote:
> This should be "UPTO_COV4". Or if we really want to say COV5, then "BEFORE_COV5" or "PRE_COV5". But to me, "UPTO_COV4" is the clearest.
This issue does not exist if the _COV5 suffix is for the type of this enum because we only consider COV5 in this definition.


================
Comment at: llvm/lib/Target/AMDGPU/SIDefines.h:784
+// Implicit kernel argument offset for code object version 5.
+enum ImplicitKernargOffset : unsigned {
+  HOSTCALL_PTR_OFFSET = 80,
----------------
arsenm wrote:
> cfang wrote:
> > arsenm wrote:
> > > Add a COV5 suffix? Probably should also wrap in a namespace
> > Add COV5 suffix, and wrap in a namespace of ImplicitArg. Rename the type to
> > Offset. So it is of AMDGPU::ImplicitArg::Offset type.  
> I meant suffix on the enum itself, not on each individual field
OK, will change to the suffix of the enum itself.


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

https://reviews.llvm.org/D120265