[PATCH] D70174: [AArch64][SVE] Correctly allocate scavenging slot in presence of SVE.

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp:327
   const AArch64FrameLowering &TFI = *getFrameLowering(MF);
-  return TFI.hasFP(MF);
+  return TFI.hasFP(MF) && !TFI.estimateSVEStackObjectOffsets(MF.getFrameInfo());
 }
----------------
sdesmalen wrote:
> efriedma wrote:
> > sdesmalen wrote:
> > > efriedma wrote:
> > > > The problem here, I guess, is the spill slot can't be addressed relative to SP? That makes sense, I guess... but do we also need to check for stack realignment here?
> > > Apologies for my delayed repsonse, I completely forgot about this patch.
> > > 
> > > > The problem here, I guess, is the spill slot can't be addressed relative to SP?
> > > In that case it will be allocated close to the BP. If there are variable-sized objects the choices are either BP or FP, otherwise the choices are SP or FP. Because the emergency spillslot needs to be accessed using the normal (non-SVE) addressing modes in one go (i.e. without requiring another register to materialize the address to the emergency slot), it needs to be able to access it directly from Pointer + <offset>, so BP or SP are really the only options. Otherwise it would need a temporary register to calculate `FP - sizeof(SVE area)`, which itself would require the emergency slot to materialise the temporary.
> > > 
> > > > do we also need to check for stack realignment here?
> > > I don't think that's specifically needed, because `hasFP()` will always evaluate to `true` if the stack requires realignment. Do you want me to add an `assert` for that?
> > I think you're misunderstanding what I'm saying about stack realignment.  I think the interaction with stack realignment is actually broken (and nobody has run into it because stack realignment is rare on AArch64).  I'm thinking that the condition here should just be `TFI.hasFP(MF) && !hasBasePointer(MF)`.
> Thanks for clarifying, I see what you mean now. The realignment case was already covered by code in PEI so it's not currently broken, but I've added the condition to this function anyway. I've also created `framelayout-scavengingslot.mir` to guard the correct behaviour.
> 
> I don't think we need to check for `!hasBasePointer(MF)` because the availability of a base pointer doesn't really matter. AIUI the FP is always desired unless there is some runtime-sized area between the FP and the emergency slot.
> ```+-----------------+
> |   Callee Saves  |
> +-----------------+ <- FP
> \ unknown size at \    |
> /  compile time   /    | (realignment gap or SVE area)
> +-----------------+    /
> |<emergency slot> | <-' This always requires more than 1 instruction from FP.
> | - - - - - - - - |  
> |     Locals      |  
> | - - - - - - - - |
> |                 | <- Better allocated here, can always access from either SP or BP.
> +-----------------+ < BP/SP
> \  [Optional VLA] \
> /                 /
> +-----------------+ < SP```
> The realignment case was already covered by code in PEI

Oh, hmm, the caller checks for stack realignment.  Makes sense, I guess.


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

https://reviews.llvm.org/D70174