[PATCH] D91121: [InferAddrSpace] Teach to handle assumed address space.

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:227
 
+  Optional<unsigned> getAssumedAddrSpace(const Value *V) const {
+    return getTLI()->getTargetMachine().getAssumedAddrSpace(V);
----------------
hliao wrote:
> hliao wrote:
> > arsenm wrote:
> > > We already have a -1 as an invalid addrspace, so optional isn't necessary
> > OK. Do we need to document that in the IR langref? That invalid address space ID is not documented anywhere.
> Shall we assume this before that reserved invalid address space ID is well-received and documented in LLVM IR ref? I will make either change in this change accordingly.
It's not really part of the IR. The address space is a 24-bit unsigned limit and this is already the pass convention


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:540
+  const auto *Ptr = LD->getPointerOperand();
+  if (Ptr->getType()->getPointerAddressSpace() != AMDGPUAS::CONSTANT_ADDRESS)
+    return None;
----------------
hliao wrote:
> arsenm wrote:
> > I think this may be too aggressive and should check if it's specifically a kernel argument load. Contant address space should also not be necessary
> If the variable locates in `__constant__` memory, it could only be modified on the host side. It's safe to assume any generic pointer loaded from `__constant__` memory is a global one.
I thought we discussed the one possible case where this wasn't necessarily true


================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:289-291
+    if (TTI->getAssumedAddrSpace(&V))
+      return true;
     return false;
----------------
This can be return TII->...


================
Comment at: llvm/test/Transforms/InferAddressSpaces/AMDGPU/assumed-addrspace.ll:12
+  ret float %v
+}
----------------
Should have test with load from kernel argument


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91121