[PATCH] D147025: [InstCombine] Teach alloca replacement to handle `addrspacecast`

[Nikita Popov via Phabricator via llvm-commits]

nikic resigned from this revision.
nikic added a comment.

I'm approving this usage of TTI in InstCombine on the following grounds:

- This is a legality query, not a profitability query. It does not impact IR canonicality in any meaningful way.
- While legality queries should really not be part of TTI, I also think that including this in DataLayout would be somewhat awkward, and making this a TTI query is the more pragmatic choice for the time being. I'd still be interested in @arsenm's opinion on whether it would make sense to make this a data layout property in the future.

I'll leave final patch review to AMDGPU maintainers, just explicitly withdrawing my objection here.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:355
       Worklist.insert(Inst);
+    } else if (isEqualOrValidAddrSpaceCast(Inst, FromAS)) {
+      Worklist.insert(Inst);
----------------
Please perform the cast to AddrSpaceCastInst here, not inside the function.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:551
+      PointerReplacer PtrReplacer(*this, AI,
+                                  TheSrc->getType()->getPointerAddressSpace());
       if (PtrReplacer.collectUsers()) {
----------------
Use `SrcAddrSpace` defined above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147025