[PATCH] D123346: AMDGPU: Align the implicit kernel argument segment to 8 bytes for v5

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:804
 
+  Offset = alignTo(Offset, ST.getAlignmentForImplicitArgPtr());
+
----------------
arsenm wrote:
> cfang wrote:
> > arsenm wrote:
> > > This isn't the real alignment. You should assert the alignment you compute using the kernarg base alignment is at least getAlignmentForImplicitArgPtr
> > I think this is to align Offset to ST.getAlignmentForImplicitArgPtr which is 8 bytes.
> > Offset = alignTo(Offset, ST.getAlignmentForImplicitArgPtr());
> > 
> > We do need this alignment to be in sync with the actual load of the implicit kernel argument.
> You do not want to use alignTo. You are not adjusting the alignment or offset here. You want to use commonAlignment(16, Offset), and assert this is >= ST.getAlignmentForImplicitArgPtr()
OK, you are re-doing the same full layout recomputation in this case, so you do need to alignTo the implicitarg align


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

https://reviews.llvm.org/D123346