[PATCH] D146031: [AMDGPU] Add MMOs for GFX11 Streamout Instructions

Thu Mar 16 01:48:09 PDT 2023

nhaehnle added inline comments.

================
Comment at: llvm/docs/AMDGPUUsage.rst:678
      Buffer Fat Pointer (experimental) 7               *TODO*
+     Streamout Registers               8               *TODO*      GS_REGS
      ================================= =============== =========== ================ ======= ============================
----------------
rovka wrote:
> foad wrote:
> > arsenm wrote:
> > > foad wrote:
> > > > arsenm wrote:
> > > > > foad wrote:
> > > > > > My gut feeling is that this should be an internal implementation detail of the compiler, should never be exposed to the user, so should not be documented here.
> > > > > > 
> > > > > > Adding @nhaehnle.
> > > > > I do think we need to document any address space numbers used for any purpose. We should also probably reserve a range of address spaces for external software uses 
> > > > I was thinking this would be completely inaccessible from IR and only introduced during instruction selection. Do you agree? Do you still think it needs to be documented here? Why?
> > > It would still need to be documented as internally used for something so it doesn't get reused for something else
> > But we can reuse the number for something else whenever we like - we can just renumber STREAMOUT_REGISTER to something different, with no externally visible effect.
> I kind of see the point that we don't really have an "LLVM IR Address Space Number" for this, since it doesn't show up in the IR. We could just document that there IS an address space for the Streamout registers (if indeed we decide we need one), but don't assign it a formal address space number in this doc.
I agree with @arsenm, we need to document our choice here to reduce the risk of conflicts.

Yes, in theory we could renumber if that happens, but isn't it better to try to prevent the problem in the first place? It's also helpful as internal documentation for anybody working on the backend.

================
Comment at: llvm/docs/AMDGPUUsage.rst:788
+**Streamout Registers**
+  Dedicated registers used by the GS NGG Streamout Instructions.
+
----------------
foad wrote:
> rovka wrote:
> > t-tye wrote:
> > > My understanding is that address spaces are about memory, not registers. So why is an address space being used for this purpose? Is it that the values are being passed in memory?
> > I think the idea is that these registers are embedded into the GDS. 
> > 
> > In principle, we could use the region address space and as far as the existing tests are concerned we'd get the same results, but the docs are pretty clear that these are not operating on GDS directly, so it seemed cleaner to have a new address space.
> > 
> > I'm open to other interpretations :)
> > My understanding is that address spaces are about memory, not registers. So why is an address space being used for this purpose? Is it that the values are being passed in memory?
> 
> They are called "registers" in the documentation but as far as the ISA is concerned they act more like memory in a separate address space that does not interact with any other type of memory. The only instructions that access them take an offset into the "register" file and read or write 32 or 64 bits at that offset. The individual registers are not named. Read operations from the register file use LGKMcnt to track when the read has completed.
> 
> I guess all of this *could* be implemented by modelling them as individual registers, but my hunch is that it would be more complicated to implement for no practical benefit.
Yeah, this is just a case of unlucky naming due to different worlds colliding. My understanding is that HW people ended up calling it "registers" because the actual HW implementation looks more like (GRBM) registers than memory, but from the shader code's perspective it very much behaves like memory.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146031