[PATCH] D151997: [AMDGPU] Document amdgpu_cs_chain[_preserve] CCs. NFC

Mon Jun 12 01:26:08 PDT 2023

nhaehnle added inline comments.

================
Comment at: llvm/docs/AMDGPUUsage.rst:1127
+
+                                     Functions must be aligned to at least 64 bytes.
+
----------------
nhaehnle wrote:
> rovka wrote:
> > nhaehnle wrote:
> > > arsenm wrote:
> > > > nhaehnle wrote:
> > > > > This shouldn't be a property of the calling convention. We should just set the alignment attribute in the frontend to ensure this.
> > > > Why? Entry points require 256 byte align but regular code is fine with 4
> > > Entry points require 256 for HW reasons. The 64 bytes we discussed is a SW choice that is orthogonal to the base functionality of amdgpu_cs_chain (making LSBs of function pointers available to stuff metadata in -- which we also could have considered doing with amdgpu_gfx functions).
> > I agree this might not be the most pertinent place to put it, but it feels like relevant information, like you said above. I don't feel very strongly about this, we could just omit it (or mention it elsewhere if you think there's a better place for it).
> Yeah, but the alignment is relevant information in the context of the specific use of this feature in LLPC. I talked with @ruiling and he's going to use Function::setAlignment to explicitly set the desired alignment. So I'd prefer to remove it here. Or perhaps rephrase it as an "FYI, there's this use of the feature which is going to set explicit alignments on the functions". But at least to me that just seems a little weird.
And I think one big reason why it feels a little weird is that there could be tons of little "FYIs" that we could add about this feature. Why is this one special? And adding all of them would just blow up this document...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151997