[RISCV] reorganize getFrameIndexReference to reduce code duplication [nfc]

[reames]

Summary

This change reorganizes the majority of frame index resolution into a two strep process.

• Step 1 - Select which base register we're going to use.
• Step 2 - Compute the offset from that base register.

The key point is that this allows us to share the step 2 logic for the SP case. This reduces the code duplication, and (I think) makes the code much easier to follow.

I also went ahead and added assertions into phase 2 to catch errors where we select an illegal base pointer. In general, we can't index from a base register to a stack location if that requires crossing a variable and unknown region. In practice, we have two such cases: dynamic stack realign and var sized objects. Note that crossing the scalable region is fine since while variable, it's a known variability which can be expressed in the offset.

After D125787, I believe this to be fully NFC. Careful review to confirm that is appreciated. :)

[StephenFan]

Thanks for refactoring it! I think it's fully NFC.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

794

Do you think it would be better to mention the BP here?

[frasercrmck]

Looks good, thank you. Should be easier to work with.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

758

A quick grep shows that starting assertion messages with lower-case letters is far less common in the RISCV backend than upper-case ones.

766

What would you say to an early return here after FrameReg == getFPReg(STI)?

794

Mentioning BP makes sense to me. But then the question may become whether we should mention that if it's BP then there's var-sized objects below it, in case people think SP always equals BP? Maybe I'm overthinking it.

[reames]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

794

This point is mentioned in the comment during base register selection. I went ahead and duplicated that here for now.

The comments have a bunch of unhelpful duplication. I'm planning a separate follow up change (with separate review) to address that.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

RISCVFrameLowering::getFrameIndexReference(const MachineFunction &MF, int FI,
 
   if (FI >= MinCSFI && FI <= MaxCSFI) {
     FrameReg = RISCV::X2;
 
     if (FirstSPAdjustAmount)
       Offset += StackOffset::getFixed(FirstSPAdjustAmount);
     else
       Offset += StackOffset::getFixed(getStackSizeWithRVVPadding(MF));
-  } else if (RI->hasStackRealignment(MF) && !MFI.isFixedObjectIndex(FI)) {
+    return Offset;
+  }
+
+  if (RI->hasStackRealignment(MF) && !MFI.isFixedObjectIndex(FI)) {
     // If the stack was realigned, the frame pointer is set in order to allow
     // SP to be restored, so we need another base register to record the stack
     // after realignment.
     // |--------------------------| -- <-- FP
     // | callee-allocated save    | | <----|
     // | area for register varargs| |      |
     // |--------------------------| |      |
     // | callee-saved registers   | |      |
if (RI->hasStackRealignment(MF) && !MFI.isFixedObjectIndex(FI)) {
     // |--------------------------| -- <-- SP
     if (hasBP(MF)) {
       FrameReg = RISCVABI::getBPReg();
     } else {
       // VarSize objects must be empty in this case!
       assert(!MFI.hasVarSizedObjects());
       FrameReg = RISCV::X2;
     }
-    // The total amount of padding surrounding RVV objects is described by
-    // RVV->getRVVPadding() and it can be zero. It allows us to align the RVV
-    // objects to the required alignment.
-    if (MFI.getStackID(FI) == TargetStackID::Default) {
-      Offset += StackOffset::getFixed(MFI.getStackSize());
-      assert(FI >= 0 && "Unhandled negative frame index");
-    } else if (MFI.getStackID(FI) == TargetStackID::ScalableVector) {
-      // Ensure the base of the RVV stack is correctly aligned: add on the
-      // alignment padding.
-      int ScalarLocalVarSize = MFI.getStackSize() -
-                               RVFI->getCalleeSavedStackSize() +
-                               RVFI->getRVVPadding();
-      Offset += StackOffset::get(ScalarLocalVarSize, RVFI->getRVVStackSize());
-    }
   } else {
     FrameReg = RI->getFrameRegister(MF);
-    if (hasFP(MF)) {
-      Offset += StackOffset::getFixed(RVFI->getVarArgsSaveSize());
-      if (FI >= 0)
-        Offset -= StackOffset::getFixed(RVFI->getLibCallStackSize());
-      // When using FP to access scalable vector objects, we need to minus
-      // the frame size.
-      //
-      // |--------------------------| -- <-- FP
-      // | callee-allocated save    | |
-      // | area for register varargs| |
-      // |--------------------------| |
-      // | callee-saved registers   | |
-      // |--------------------------| | MFI.getStackSize()
-      // | scalar local variables   | |
-      // |--------------------------| -- (Offset of RVV objects is from here.)
-      // | RVV objects              |
-      // |--------------------------|
-      // | VarSize objects          |
-      // |--------------------------| <-- SP
-      if (MFI.getStackID(FI) == TargetStackID::ScalableVector) {
-        // We don't expect any extra RVV alignment padding, as the stack size
-        // and RVV object sections should be correct aligned in their own
-        // right.
-        assert(MFI.getStackSize() == getStackSizeWithRVVPadding(MF) &&
-               "Inconsistent stack layout");
-        Offset -= StackOffset::getFixed(MFI.getStackSize());
-      }
-    } else {
-      // When using SP to access frame objects, we need to add RVV stack size.
-      //
-      // |--------------------------| -- <-- FP
-      // | callee-allocated save    | | <----|
-      // | area for register varargs| |      |
-      // |--------------------------| |      |
-      // | callee-saved registers   | |      |
-      // |--------------------------| --     |
-      // | RVV alignment padding    | |      |
-      // | (not counted in          | |      |
-      // | MFI.getStackSize() but   | |      |
-      // | counted in               | |      |
-      // | RVFI.getRVVStackSize())  | |      |
-      // |--------------------------| --     |
-      // | RVV objects              | |      |-- MFI.getStackSize()
-      // | (not counted in          | |      |
-      // | MFI.getStackSize())      | |      |
-      // |--------------------------| --     |
-      // | padding before RVV       | |      |
-      // | (not counted in          | |      |
-      // | MFI.getStackSize())      | |      |
-      // |--------------------------| --     |
-      // | scalar local variables   | | <----'
-      // |--------------------------| -- <-- SP
-      //
-      // The total amount of padding surrounding RVV objects is described by
-      // RVV->getRVVPadding() and it can be zero. It allows us to align the RVV
-      // objects to the required alignment.
-      if (MFI.getStackID(FI) == TargetStackID::Default) {
-        if (MFI.isFixedObjectIndex(FI)) {
-          Offset += StackOffset::get(getStackSizeWithRVVPadding(MF) +
-                                         RVFI->getLibCallStackSize(),
-                                     RVFI->getRVVStackSize());
-        } else {
-          Offset += StackOffset::getFixed(MFI.getStackSize());
-        }
-      } else if (MFI.getStackID(FI) == TargetStackID::ScalableVector) {
-        // Ensure the base of the RVV stack is correctly aligned: add on the
-        // alignment padding.
-        int ScalarLocalVarSize =
-            MFI.getStackSize() - RVFI->getCalleeSavedStackSize() -
-            RVFI->getVarArgsSaveSize() + RVFI->getRVVPadding();
-        Offset += StackOffset::get(ScalarLocalVarSize, RVFI->getRVVStackSize());
-      }
-    }
-  }
-
+  }
+
+  if (FrameReg == getFPReg(STI)) {
+      assert(!RI->hasStackRealignment(MF) &&
+             "Can't index across variable sized realign");
+    return Offset;
+  }
+
+  // This case handles indexing off both SP and BP.
+  // If indexing off SP, there must not be any var sized objects
+  assert(FrameReg == RISCVABI::getBPReg() || !MFI.hasVarSizedObjects());
+
+  // |--------------------------| -- <-- BP (if var sized objects present)
+  // | VarSize objects          | |
+      assert(!RI->hasStackRealignment(MF) &&
+             "Can't index across variable sized realign");
   return Offset;
 }
 
 void RISCVFrameLowering::determineCalleeSaves(MachineFunction &MF,
                                               BitVector &SavedRegs,
                                               RegScavenger *RS) const {
   TargetFrameLowering::determineCalleeSaves(MF, SavedRegs, RS);
   // Unconditionally spill RA and FP only if the function uses a frame