[PATCH] D45228: AMDGPU/SI: Handle BitCast of GEP in promoting alloca to vector

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:330
     LoadInst *LI = cast<LoadInst>(Inst);
-    // Currently only handle the case where the Pointer Operand is a GEP so check for that case.
-    return isa<GetElementPtrInst>(LI->getPointerOperand()) && !LI->isVolatile();
+    if (LI->isVolatile())
+      return false;
----------------
I think there's another bug here (which should be fixed in a separate patch). This probably also needs to check if it's atomic


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:340-341
+  }
+  case Instruction::BitCast: {
+    if (isa<GetElementPtrInst>(Used)) {
+      for (User *BCUser : Inst->users()) {
----------------
This shouldn't need to special case GEP users. You really want something like stripPointerCasts, but not one that looks through addrspacecast.

Could use a test with 0 index GEPs which should show the same problem.


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:348
+    }
+    // Fallthrough othereise.
+    // TODO: we do not actually have logic to handle general bitcast and
----------------
Typo othereise


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:354
   case Instruction::AddrSpaceCast:
     return true;
   case Instruction::Store: {
----------------
This looks broken. addrspacecast should forbid the vector promotion. It should only be allowed for the LDS promotion. Separate patch?


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:357
     StoreInst *SI = cast<StoreInst>(Inst);
-    return (SI->getPointerOperand() == User) && isa<GetElementPtrInst>(User) && !SI->isVolatile();
+    if (SI->isVolatile())
+      return false;
----------------
Same as with the load


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:433-436
+      if (isa<BitCastInst>(Ptr)) {
+        VectorTy = VectorType::get(Ptr->getType()->getPointerElementType(),
+                                   AllocaTy->getNumElements());
+        Ptr = cast<BitCastInst>(Ptr)->getOperand(0);
----------------
Use dyn_cast instead of separate isa and cast


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:452
+      VectorType *VectorTy = VectorT;
+      if (isa<BitCastInst>(Ptr)) {
+        VectorTy = VectorType::get(Ptr->getType()->getPointerElementType(),
----------------
Same as the load case


================
Comment at: test/CodeGen/AMDGPU/vector-alloca.ll:154
+  %gep_write = getelementptr inbounds [3 x i32], [3 x i32] addrspace(5)* %scratch, i32 0, i32 %w_index
+  %bc_write = bitcast i32 addrspace(5)* %gep_write to float addrspace(5)*
+  store float 12.0, float addrspace(5)* %bc_write, align 4
----------------
needs some more tests with multiple uses of the bitcast source, with accesses through different types as well as non-bitcast users


https://reviews.llvm.org/D45228