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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/docs/AMDGPUUsage.rst:678
      Buffer Fat Pointer (experimental) 7               *TODO*
+     Streamout Registers               8               *TODO*      GS_REGS
      ================================= =============== =========== ================ ======= ============================
----------------
scott.linder wrote:
> nhaehnle wrote:
> > 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.
> I would also tend to agree that we should document it here, as the alternative is more confusing (i.e. seeing it documented but not needing it is worse than noticing it exists and finding no documentation).
> 
> The fact that it currently exists only transitively during ISEL also seems like an implementation detail, and so I don't think it must dictate whether we document it.
> seeing it documented but not needing it
The risk is seeing it documented and trying to access it from IR and finding that that "doesn't work".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146031