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

Wed Mar 22 02:40:34 PDT 2023

foad 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:
> > scott.linder wrote:
> > > foad wrote:
> > > > 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".
> > > We can document it as we already do Generic, stating when it is meaningful. Or we can reserve a range of values for "internal" purposes and document them in another table, so there is no chance for confusion?
> > Sure, if the documentation is clear then I have no objection to documenting it.
> > 
> > Do you know what the failure mode is if you try to compile IR that uses an invalid or unsupported address space?
> I think it only fails during instruction selection. 
> At the IR level we don't seem to have a problem with unknown address spaces, in fact there's been [[ https://github.com/llvm/llvm-project/commit/78a43f10c701a9cb353e927e9551f32223fbb37d | some effort ]] to let them just pass through. We have a handful of tests using addrspace(999), e.g. to check that [[ https://github.com/llvm/llvm-project/blob/9297b9f8eeecc5ea6571cf45985ba77bc2960427/llvm/test/CodeGen/AMDGPU/amdgpu-alias-analysis.ll#L46 | AA treats it conservatively ]].
> 
> In our case, I could probably teach AMDGPU AA that calls to these GS intrinsics don't affect any pointers (WIP), but I'm not sure what we ought to do about IR pointers to internal address spaces. Should we error out anywhere before instruction selection, or just leave them undisturbed through the middle-end and ICE later on in isel? Should we check this in the IRVerifier? Other thoughts?
"failed to select" seems like a reasonable failure mode for now.

Adding some kind of checks in the IR verifier sounds nice but I don't know what would be appropriate there. Do we even have a notion of which address spaces are valid for a particular target?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146031