[RISCV][NFCI] Set TransientStackAlignment and rely on it rather than RVV-specific logic on RVV-less functions

[asb]

Summary

This should be a fairly simple refactoring, but anything related to stack frames is fiddly enough I feel it's worth at least a quick review.

• TargetFrameLowering has a TransientStackAlignment field that "returns the number of bytes to which the stack pointer must be aligned at all times, even between calls.
  • As explained in the RISC-V calling convention, the stack pointer must remain fully aligned throughout execution for compliant code. This is important for embedded targets that might avoid realigning the stack pointer for interrupt service routines. Systems running full OSes may always realign the stack anyway. I've added a FIXME that we may want to revisit it.
• TransientStackAlignment is used in estimateStackSize in MachineFrameInfo and in PEI::calculateFrameObjectOffsets.
  • estimateStackSize is only used in the RISC-V backend for scavenging slots. It may be possible to craft a function where the difference is observable, but it wouldn't be a meaningful test.
  • calculateFrameObjectOffsets makes use of TransientStackAlignment, but then sets the stack alignment to the max of that alignment and MaxAlign, which is unconditionally set to 16 in RISCVFrameLowering::processFunctionBeforeFrameFinalized
  • I've changed this logic to only set MaxAlign if there are RVV frame objects. There should be no functional change here for either RVV targets (MaxAlign is set as before) or non-RVV targets (TransientStackAlign is now 16 anyway).

I could split this into separate patches for TransientStackAlign and the processFunctionBeforeFrameFinalized change if desired.

[frasercrmck]

LGTM, thanks for the well-written description.

[asb]

Thanks for the review @frasercrmck. As I'd hate to add a misleading comment to the codebase, perhaps someone (@jrtc27, @kito-cheng perhaps?) could confirm if my understanding that TransientStackAlignment is unlikely to be needed on systems running e.g. Linux or FreeBSD is correct? i.e. they'll always realign or use a separate stack in any interrupt handlers?

[asb]

In D130068#3667806, @asb wrote:

Thanks for the review @frasercrmck. As I'd hate to add a misleading comment to the codebase, perhaps someone (@jrtc27, @kito-cheng perhaps?) could confirm if my understanding that TransientStackAlignment is unlikely to be needed on systems running e.g. Linux or FreeBSD is correct? i.e. they'll always realign or use a separate stack in any interrupt handlers?

Friendly to @jrtc27 @kito-cheng on this point. Thanks in advance.

[kito-cheng]

Leave some comment before but apparently I never click submit.

---

Traced related implementation in LLVM, the comment say it means the number of bytes to stack alignment all the time[1], so according the description this should always set to the stack alignment which psABI required no matter it's baremetal or not:

• Linux require this be true since the stack space of signal handler will based on the current stack, and signal could be happened in any time, so the stack must align to ABI stack alignment.
• Bare-metal might also use stack pointer directly in the interrupter handler just like signal handler, so that's same situation as signal handler, we need always make sure this align to ABI stack alignment.

So LGTM, but I think the FIXME could be removed since I think it's also true for non-baremetal - at least for Linux (I don't know too much about other OS like FreeBSD).

[1]
llvm/include/llvm/CodeGen/TargetFrameLowering.h

/// getTransientStackAlignment - This method returns the number of bytes to
/// which the stack pointer must be aligned at all times, even between
/// calls.

[kito-cheng]

More info about linux signal behavior:

Default stack based on normal process stack, but could setup a separated stack if requested

Ref:
https://man7.org/linux/man-pages/man7/signal.7.html

...
       By default, a signal handler is invoked on the normal process
       stack.  It is possible to arrange that the signal handler uses an
       alternate stack; see sigaltstack(2) for a discussion of how to do
       this and when it might be useful.
...

[asb]

Thanks Kito - I'll drop the FIXME and land this.

llvm/lib/Target/RISCV/RISCVFrameLowering.h

 #include "llvm/CodeGen/TargetFrameLowering.h"
 #include "llvm/Support/TypeSize.h"
 
 namespace llvm {
 class RISCVSubtarget;
 
 class RISCVFrameLowering : public TargetFrameLowering {
 public:
+  // FIXME: Setting TransientStackAlignment is likely to be unnecessary on
+  // platforms that aren't bare metal.
   explicit RISCVFrameLowering(const RISCVSubtarget &STI)
       : TargetFrameLowering(StackGrowsDown,
                             /*StackAlignment=*/Align(16),
-                            /*LocalAreaOffset=*/0),
+                            /*LocalAreaOffset=*/0,
+                            /*TransientStackAlignment=*/Align(16)),
         STI(STI) {}
 
   void emitPrologue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
   void emitEpilogue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
 
   uint64_t getStackSizeWithRVVPadding(const MachineFunction &MF) const;
 
   StackOffset getFrameIndexReference(const MachineFunction &MF, int FI,

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

for (const MachineInstr &MI : MBB) {
         }
       }
       if (MaxScavSlotsNum == MaxScavSlotsNumKnown)
         return MaxScavSlotsNumKnown;
     }
   return MaxScavSlotsNum;
 }
 
+static bool hasRVVFrameObject(const MachineFunction &MF) {
+  // Originally, the function will scan all the stack objects to check whether
+  // if there is any scalable vector object on the stack or not. However, it
+  // causes errors in the register allocator. In issue 53016, it returns false
+  // before RA because there is no RVV stack objects. After RA, it returns true
+  // because there are spilling slots for RVV values during RA. It will not
+  // reserve BP during register allocation and generate BP access in the PEI
+  // pass due to the inconsistent behavior of the function.
+  //
+  // The function is changed to use hasVInstructions() as the return value. It
+  // is not precise, but it can make the register allocation correct.
+  //
+  // FIXME: Find a better way to make the decision or revisit the solution in
+  // D103622.
+  //
+  // Refer to https://github.com/llvm/llvm-project/issues/53016.
+  return MF.getSubtarget<RISCVSubtarget>().hasVInstructions();
+}
+
 void RISCVFrameLowering::processFunctionBeforeFrameFinalized(
     MachineFunction &MF, RegScavenger *RS) const {
   const RISCVRegisterInfo *RegInfo =
       MF.getSubtarget<RISCVSubtarget>().getRegisterInfo();
   MachineFrameInfo &MFI = MF.getFrameInfo();
   const TargetRegisterClass *RC = &RISCV::GPRRegClass;
   auto *RVFI = MF.getInfo<RISCVMachineFunctionInfo>();
 
   int64_t RVVStackSize;
   Align RVVStackAlign;
   std::tie(RVVStackSize, RVVStackAlign) = assignRVVStackObjectOffsets(MF);
 
   RVFI->setRVVStackSize(RVVStackSize);
   RVFI->setRVVStackAlign(RVVStackAlign);
 
+  if (hasRVVFrameObject(MF)) {
     // Ensure the entire stack is aligned to at least the RVV requirement: some
     // scalable-vector object alignments are not considered by the
     // target-independent code.
     MFI.ensureMaxAlignment(RVVStackAlign);
+  }
 
   // estimateStackSize has been observed to under-estimate the final stack
   // size, so give ourselves wiggle-room by checking for stack size
   // representable an 11-bit signed field rather than 12-bits.
   // FIXME: It may be possible to craft a function with a small stack that
   // still needs an emergency spill slot for branch relaxation. This case
   // would currently be missed.
   // RVV loads & stores have no capacity to hold the immediate address offsets
for (const auto &Info : MFI.getCalleeSavedInfo()) {
     if (MFI.getStackID(FrameIdx) != TargetStackID::Default)
       continue;
 
     Size += MFI.getObjectSize(FrameIdx);
   }
   RVFI->setCalleeSavedStackSize(Size);
 }
 
-static bool hasRVVFrameObject(const MachineFunction &MF) {
-  // Originally, the function will scan all the stack objects to check whether
-  // if there is any scalable vector object on the stack or not. However, it
-  // causes errors in the register allocator. In issue 53016, it returns false
-  // before RA because there is no RVV stack objects. After RA, it returns true
-  // because there are spilling slots for RVV values during RA. It will not
-  // reserve BP during register allocation and generate BP access in the PEI
-  // pass due to the inconsistent behavior of the function.
-  //
-  // The function is changed to use hasVInstructions() as the return value. It
-  // is not precise, but it can make the register allocation correct.
-  //
-  // FIXME: Find a better way to make the decision or revisit the solution in
-  // D103622.
-  //
-  // Refer to https://github.com/llvm/llvm-project/issues/53016.
-  return MF.getSubtarget<RISCVSubtarget>().hasVInstructions();
-}
-
 // Not preserve stack space within prologue for outgoing variables when the
 // function contains variable size objects or there are vector objects accessed
 // by the frame pointer.
 // Let eliminateCallFramePseudoInstr preserve stack space for it.
 bool RISCVFrameLowering::hasReservedCallFrame(const MachineFunction &MF) const {
   return !MF.getFrameInfo().hasVarSizedObjects() &&
          !(hasFP(MF) && hasRVVFrameObject(MF));
 }