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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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());
----------------
dstuttard wrote:
> 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?
Yes


================
Comment at: lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:432
       AllocaTy->getElementType()->isVectorTy() ||
+      AllocaTy->getElementType()->isArrayTy() ||
       AllocaTy->getNumElements() > 4 ||
----------------
dstuttard wrote:
> 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?
Yes


https://reviews.llvm.org/D31710