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

[Matt Arsenault via Phabricator via llvm-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);
----------------
We already have a -1 as an invalid addrspace, so optional isn't necessary


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:533
+  auto *Ty = V->getType();
+  if (!Ty->isPointerTy() ||
+      Ty->getPointerAddressSpace() != AMDGPUAS::FLAT_ADDRESS)
----------------
I would assume !isPointerTy would be unreachable


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:540
+  const auto *Ptr = LD->getPointerOperand();
+  if (Ptr->getType()->getPointerAddressSpace() != AMDGPUAS::CONSTANT_ADDRESS)
+    return None;
----------------
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


================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:563-571
+  if (auto AS = TTI->getAssumedAddrSpace(I)) {
+    // For the assumed address space, insert an `addrspacecast` to make that
+    // explicit.
+    auto *NewPtrTy =
+        I->getType()->getPointerElementType()->getPointerTo(AS.getValue());
+    auto *NewI = new AddrSpaceCastInst(I, NewPtrTy);
+    NewI->insertAfter(I);
----------------
Won't this cast be naturally inserted with the inferred addres space?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91121