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

[Michael Liao via Phabricator via cfe-commits]

hliao marked 3 inline comments as done.
hliao added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:227
 
+  Optional<unsigned> getAssumedAddrSpace(const Value *V) const {
+    return getTLI()->getTargetMachine().getAssumedAddrSpace(V);
----------------
arsenm wrote:
> 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
You are right. Forget that totally.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:540
+  const auto *Ptr = LD->getPointerOperand();
+  if (Ptr->getType()->getPointerAddressSpace() != AMDGPUAS::CONSTANT_ADDRESS)
+    return None;
----------------
arsenm wrote:
> 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
For `__constant__` memory, the language itself guarantees it won't be changed on the device side. It's very safe to assume that. The frontend should issue an error on any attempt to write it. Also, the runtime and HW is supposed to provide the facility to protect any write into that space from the device side.

I thought the issue we may have the issue is the case for `const` variable in the global memory space and a `const` global pointer in the kernel argument. We need to investigate them further to identify potential risks.


================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:289-291
+    if (TTI->getAssumedAddrSpace(&V))
+      return true;
     return false;
----------------
arsenm wrote:
> This can be return TII->...
good catch. The's the result of the previous Optional<unsigned>. Now, it's revised.


================
Comment at: llvm/test/Transforms/InferAddressSpaces/AMDGPU/assumed-addrspace.ll:12
+  ret float %v
+}
----------------
arsenm wrote:
> Should have test with load from kernel argument
most cases are already captured in the clang test. A new function is added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91121