[PATCH] D140599: AMDGPU: Promote array alloca if used by memmove/memcpy

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added a comment.

Thanks for the careful review @arsenm, I have fixed most of them. Please help take a second look. Thanks!



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:502
+
+      auto getPointerIndexOfAlloca = [&](Value *Ptr) -> ConstantInt * {
+        GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Ptr);
----------------
arsenm wrote:
> [=]?
We are capturing `GEPVectorIdx` and `Alloca`, so I think capture by reference is preferred over by copy here. Please correct me if I am wrong.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:593-595
+          case Intrinsic::memcpy:
+          case Intrinsic::memcpy_inline:
+          case Intrinsic::memmove: {
----------------
arsenm wrote:
> probably should dyn_cast to MemIntrinsicInst here to defend agains these becoming out of sync
Agree, I have changed to cast to catch the possible broken case.


================
Comment at: llvm/test/CodeGen/AMDGPU/promote-alloca-array-aggregate.ll:219
+
+declare void @llvm.memcpy.p5i8.p5i8.i32(ptr addrspace(5) nocapture writeonly, ptr addrspace(5) nocapture readonly, i32, i1 immarg)
+declare void @llvm.memmove.p5i8.p5i8.i32(ptr addrspace(5) nocapture writeonly, ptr addrspace(5) nocapture readonly, i32, i1 immarg)
----------------
arsenm wrote:
> arsenm wrote:
> > Needs tests for the different address space cases you mentioned in the todo 
> also memcpy_inline
Good idea, I have added all of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140599