[PATCH] D145586: [AMDGPU] Tweak PromoteAlloca limits

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:408
+  const unsigned SizeFactor =
+      ((MaxVGPRs >= 64 && !PromoteAllocaToVectorLimit) ? 2 : 4);
+  if (DL.getTypeSizeInBits(AllocaTy) * SizeFactor > Limit) {
----------------
It does not make sense to me to do things differently if -amdgpu-promote-alloca-to-vector-limit is used. This option is not for users, it is for us to debug the pass and experiment. Changing pass behavior while debugging does not help.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:181
-    MaxVGPRs = ST.getMaxNumVGPRs(ST.getWavesPerEU(F).first);
-    // A non-entry function has only 32 caller preserved registers.
-    // Do not promote alloca which will force spilling.
----------------
Pierre-vh wrote:
> rampitec wrote:
> > arsenm wrote:
> > > rampitec wrote:
> > > > Isn't this still true? You might not see alloca, but you will see spills.
> > > Trading spills inside a function for CSR spills can be profitable, such as if the spilling occurs in a loop.
> > > 
> > > IIRC this was to workaround some compile failures 
> > One reason for the current limits were 'run of registers' errors in some cases. I'd be really careful extending the limits.
> I would be okay with leaving the limit at 1/4 (or raising it to 1/3 instead of 1/2), as long as the entry function CC exception is removed. That should be enough for the particular case I'm interested in.
> I would be okay with leaving the limit at 1/4 (or raising it to 1/3 instead of 1/2), as long as the entry function CC exception is removed. That should be enough for the particular case I'm interested in.

If Matt is OK trading alloca to spilling let it be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145586