[PATCH] D31710: [AMDGPU] Fix for issue in alloca to vector promotion pass

[David Stuttard via Phabricator via llvm-commits]

dstuttard added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:405-407
+    // Canonical form means that the pointer should be a GEP
+    LoadInst *LI = cast<LoadInst>(Inst);
+    return isa<GetElementPtrInst>(LI->getPointerOperand());
----------------
arsenm wrote:
> This isn't necessarily true. You could have a direct store to an alloca pointer for example
Ok - but since the current implementation expects a gep that's what is being verified here.
Would a comment change be sufficient?


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:432
       AllocaTy->getElementType()->isVectorTy() ||
+      AllocaTy->getElementType()->isArrayTy() ||
       AllocaTy->getNumElements() > 4 ||
----------------
arsenm wrote:
> I can see how this would be a problem and not handled today, but I don't think anything flatten array types. You could still see something like [4 x [4 x i32]], though the elements will still be individually addressed, not as aggregate loads and stores
Yes you're correct. It should be possible to extend this routine to handle (for instance) [2 x [2 x i32]] and still fit in the current size constraints.
The current implementation won't handle this though.
Perhaps a FIXME or TODO comment to that effect in here?


https://reviews.llvm.org/D31710