[PATCH] D20297: AMDGPU/SI: Make kernarg.segment.ptr point to implicit arguments for non HSA

[Tom Stellard via llvm-commits]

tstellarAMD added a comment.

In http://reviews.llvm.org/D20297#431404, @arsenm wrote:

> In http://reviews.llvm.org/D20297#431388, @jvesely wrote:
>
> > In http://reviews.llvm.org/D20297#431315, @arsenm wrote:
> >
> > > In http://reviews.llvm.org/D20297#431298, @jvesely wrote:
> > >
> > > > In http://reviews.llvm.org/D20297#431268, @arsenm wrote:
> > > >
> > > > > We don't actually want these. We now have the kernarg.segment.ptr intrinsic, so the library should just directly read the offsets from there
> > > >
> > > >
> > > > I assume it points to the beginning of the kernel args.
> > > >  Is the idea to have another intrinsic to read from the end (workdim, global offset), or is it OK to have intrinsics for those?
> > >
> > >
> > > We should fix clover to not read from the end of the arguments (I thought this is what it already did)?
> >
> >
> > some are before the kernel args (global size, local size, ngroups -- this is done by radeonsi/r600 mesa driver). others are after the kernel arguments (work_dim, global-offset -- this is done by clover).
> >  The intention was to move all implicit arguments after the explicit ones, so new implicit arguments can be added without breaking ABI (moving explicit arguments).
>
>
> I think clover should move towards matching the HSA ABI closer. Most of the implicit arguments would then be user SGPR inputs like HSA uses, and the number of implicit args would be reduced.


I agree.  I actually started working on this last week.  I think all implicit args that aren't passed in SGPRs should be at the end of the kernarg segment.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:1574-1579
@@ -1567,2 +1573,8 @@
   case Intrinsic::amdgcn_kernarg_segment_ptr: {
+    if (!Subtarget->isAmdHsaOS()) {
+      unsigned offset = getImplicitParameterOffset(MFI, GRID_DIM);
+      llvm::dbgs() << "FOUND offset of the first implicit arg " << offset << "\n";
+      return LowerParameterPtr(DAG, DL, DAG.getEntryNode(),
+                               getImplicitParameterOffset(MFI, GRID_DIM));
+    }
     unsigned Reg
----------------
There should be a separate intrinsic which points to the start of implicit args.  As the ABI is currently, the library could use kernarg_segment_ptr to load the workgroup size information.


Repository:
  rL LLVM

http://reviews.llvm.org/D20297