This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Correctly allocate scavenging slot in presence of SVE.
ClosedPublic

Authored by sdesmalen on Nov 13 2019, 5:06 AM.

Details

Summary

This patch addresses two issues:

  • Forces the availability of the base-pointer (x19) when the frame has both scalable vectors and variable-length arrays. Otherwise it will be expensive to access non-SVE locals.
  • In presence of SVE stack objects, it will allocate the emergency scavenging slot close to the SP, so that they can be accessed from the SP or BP if available. If accessed from the frame-pointer, it will otherwise need an extra register to access the scavenging slot because of mixed scalable/non-scalable addressing modes.

Diff Detail

Event Timeline

sdesmalen created this revision.Nov 13 2019, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 13 2019, 5:06 AM
efriedma added inline comments.Nov 13 2019, 12:41 PM
llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
405–409

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?

This fixes an issue building SLEEF with SVE at -O0. It needs rebasing though as it doesn't apply cleanly to HEAD.

fpetrogalli added inline comments.
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
2522 ↗(On Diff #229064)

I think you should update the description of the method to reflect the new interface.

2584–2585 ↗(On Diff #229064)

I think you can remove these two and pass them directly inside the call as [](int, int64_t){} and [](int, unsigned){}

You could actually make these two values the default argument of the method, so you don't have to bother specifying them here? (up to you, though)

2592–2598 ↗(On Diff #229064)

Nit: these should also be passed directly as argument to the call, no need to set up variables that are not used anywhere else.

I say this is a nit because the first lambda is 4 lines long, so it might not look beautiful once placed inside the function parameters... :)

sdesmalen updated this revision to Diff 278804.Jul 17 2020, 9:32 AM
sdesmalen marked 4 inline comments as done.
  • Rebased patch.
  • Updated framelayout-sve.mir
  • Addressed comments.
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
2592–2598 ↗(On Diff #229064)

Yes this was deliberate as I think it reads a bit easier than one big expression.

llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
405–409

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?

efriedma added inline comments.Jul 20 2020, 2:28 PM
llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
405–409

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).

sdesmalen updated this revision to Diff 279479.Jul 21 2020, 4:20 AM
sdesmalen marked an inline comment as done.
sdesmalen retitled this revision from [AArch64][SVE] Use FP for scavenging slot to [AArch64][SVE] Correctly allocate scavenging slot in presence of SVE..
sdesmalen edited the summary of this revision. (Show Details)
  • Changed condition to useFPForScavengingIndex
  • Added new test framelayout-scavengingslot.mir to guard that stack realignment (without SVE) allocates the scavenging slot in the right place.
  • Simplified patch, no need to refactor estimateSVEStackObjectOffsets
  • Renamed misleading/wrong title of this patch.
sdesmalen marked an inline comment as done.Jul 21 2020, 4:44 AM
sdesmalen added inline comments.
llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
405–409

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
efriedma accepted this revision.Jul 21 2020, 2:51 PM

LGTM

llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
405–409

The realignment case was already covered by code in PEI

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

This revision is now accepted and ready to land.Jul 21 2020, 2:51 PM
This revision was automatically updated to reflect the committed changes.