[PATCH] D155699: [AMDGPU] Allow vector access types in PromoteAllocaToVector

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:415
+    if (isa<FixedVectorType>(AccessTy)) {
+      assert(AccessSize % (DL.getTypeStoreSize(VecEltTy)) == 0);
+      const unsigned NumElts = AccessSize / DL.getTypeStoreSize(VecEltTy);
----------------
extra parens around getTypeStoreSize


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:468
+    if (isa<FixedVectorType>(AccessTy)) {
+      assert(AccessSize % (DL.getTypeStoreSize(VecEltTy)) == 0);
+      const unsigned NumElts = AccessSize / DL.getTypeStoreSize(VecEltTy);
----------------
Extra parens


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:474
+
+      // insert_vector is only legal if (Index % NumElts == 0)
+      // If we can't use it, use insertelements.
----------------
Is there a benefit to using it here? I'd assume we end up breaking it down anyway during legalization


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:557
+    const unsigned VecEltSize = DL.getTypeStoreSize(VecTy->getElementType());
+    return (AccessVecSize % VecEltSize) == 0;
+  }
----------------
These actually return TypeSize, you can do 
DL.getTypeStoreSize(AccessTy).isKnownMultipleOf(DL.getTypeStoreSize(VecTy->getElementType()))


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:560
+
+  if (CastInst::isBitOrNoopPointerCastable(VecTy->getElementType(), AccessTy,
+                                           DL))
----------------
return isBitOrNoop...


================
Comment at: llvm/test/CodeGen/AMDGPU/promote-alloca-subvecs.ll:46
+}
+
+define void @test_different_type_subvector(<4 x i32> %val.0, <8 x i16> %val.1, <16 x i8> %val.2) {
----------------
Try some 1 x vectors, those usually break something.

Also vectors of pointers, and FP types


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155699