[PATCH] D145586: [AMDGPU] Tweak PromoteAlloca limits

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:420-425
   // could also be promoted but we don't currently handle this case
   if (!VectorTy || VectorTy->getNumElements() > 16 ||
       VectorTy->getNumElements() < 2) {
     LLVM_DEBUG(dbgs() << "  Cannot convert type to vector\n");
     return false;
   }
----------------
arsenm wrote:
> This whole size logic doesn't make sense when viewed in total. The element count check should be considered along with the size. I also don't think using fractions of the register budget makes sense when deciding for an individual alloca. I think it would be easier to follow if we used the 32-bit element count limit as 16 for budgets < 64, and 32 for > 64.
> 
> Thinking about byte sizes doesn't really make sense either. For sub-32-bit elements, don't we end up promoting them to 32-bit element vectors anyway?
I'm not sure i understand, do you mean that for 32-bit elements and up we should use `(ByteLimit < 64) ? 16 : 32` as the maximum number of elements, and something different for <32-bit element arrays?

Note that the current patch, as is, doesn't change the limit other than removing the CC exception + adding some NFC changes.
As this is blocking an issue it'd be nice if we could land the minimum changes needed to fix it soon; then I can make another patch to make bigger changes to how the limit is decided. Would that work for you? I could also remove some changes from this patch to make it strictly about removing the CC limit;

FWIW, I was planning to do a big set of changes to this pass soon™  starting with changing its structure so it builds a worklist before doing the transformation so we can consider the function as a whole. If that goes through then whatever limit we set here may get removed anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145586