[PATCH] D78811: [AMDGPU] Enable base pointer.

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas marked an inline comment as done.
cdevadas added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:118
+  // stack pointer, so we reserve a base pointer.
+  return needsStackRealignment(MF);
+}
----------------
arsenm wrote:
> cdevadas wrote:
> > arsenm wrote:
> > > cdevadas wrote:
> > > > scott.linder wrote:
> > > > > Is it possible to have some mechanism for only setting up the BP when it is actually used? Looking at X86 it seems like they don't do this, so I am OK with us not doing it either, just wondering if it is feasible.
> > > > It sounds good to me. We can enable it only when there is a fixed-frame. 
> > > > It can be easily done with an additional check, MFI.getNumFixedObjects() != 0
> > > I think getNumFixedObjects is not the right check here. That won't deal with the case where we have dead stack objects (as is common from eliminated SGPR spils)
> > It would be good to have a straight forward check here to see if there is any fixed-frame and getNumFixedObjects would do that.
> > 
> >  
> It doesn't, because for us it's full of litter from SGPR spills. This will be overly conservative, and trigger when we have no actual stack objects. Is this only called after the frame is finalized?
> 
> The way the x86 version expresses this makes more sense to me:
> ister.
>    bool CantUseFP = needsStackRealignment(MF);
>    return CantUseFP && CantUseSP(MFI);
> 
> This might work for us the same?
There are calls to hasBasePointer() early during SIIselLowering and it is not always called after the frame is finalized.

It doesn't work in the way that we wanted if we include the function CantUseSP().
The condition 'CantUseFP && CantUseSP' 
enables BP when there is dynamic re-align and have either variable-sized objects or opaque SP adjustment.
The dynamic realign alone doesn't qualify to have BP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78811