[RISCV] Add clarifying asserts to getFrameIndexReference [NFC]

[reames]

Summary

This reorganizes comments slightly (to make the key difference between two big blocks more obvious), and adds some clarifying asserts. This is motivated by me trying to wrap my head around the code enough to usefully review D125787

[jrtc27]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

718

The whole point of getFrameRegister is to avoid hard-coding, and this would be a pain for us downstream in CHERI LLVM where pure-capability code uses capability register C8 not the X8 integer sub-register. I mean, I'd just delete the assert downstream rather than repeat the condition... so don't like this assert.

[reames]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

718

Would having a RISCVABI::getFPReg() and using that in the assert make life easier for you? It would still give me the property I want to assert (e.g. we are using FP if hasFP is true), but maybe be a bit easier to merge?

[jrtc27]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

718

We already have getFP/SPReg functions in this file that take the subtarget. Using those for this would be fine by me.

[reames]

Address review comment

[StephenFan]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

718

I also don't like this assertion and IMO we need to avoid this trivial assertion. I call it a trivial assertion since getFrameRegister would return the frame register according to the result of hasFP, this assertion would always succeed unless the compiler generates the wrong code.

719

Does if present means if the BP exists? But If both realignment and VarSize objects exist, then BP must be present.

[reames]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

718

Unless you can point me to a comment that says that's the document semantics of getFrameRegister, I disagree this is a trivial assert.

Going even further, it's not clear to me that the getFrameRegister actually even implements the contract its supposed to. Per the header comment in TargetRegisterInfo.h, it seems this should be returning BP in the cases that's the register we index off. However, that's *definitely* a separate patch.

719

Please see the code immediately below this. If you have a suggestion on the comment wording, feel free to suggest something.

[StephenFan]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

696

Remove the curly bracket?

719

Oh, I see. Now it's ok. Thanks

[reames]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

696

We generally prefer consistency between if and else. The else block requires the bracket.

[reames]

Rebase, and drop frame register asserts. I'll move those into a separate commit, but they're slowing down the more useful part of the patch.

[frasercrmck]

LGTM, thanks. I think this is easier to understand.

[StephenFan]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

696

Makes sense to me.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

if (FI >= MinCSFI && FI <= MaxCSFI) {
     FrameReg = RISCV::X2;
 
     if (FirstSPAdjustAmount)
       Offset += StackOffset::getFixed(FirstSPAdjustAmount);
     else
       Offset +=
           StackOffset::getFixed(MFI.getStackSize() + RVFI->getRVVPadding());
   } else 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.
-    if (hasBP(MF)) {
-      FrameReg = RISCVABI::getBPReg();
-      // |--------------------------| -- <-- FP
-      // | callee-allocated save    | | <----|
-      // | area for register varargs| |      |
-      // |--------------------------| |      |
-      // | callee-saved registers   | |      |
-      // |--------------------------| --     |
-      // | realignment (the size of | |      |
-      // | this area is not counted | |      |
-      // | in MFI.getStackSize())   | |      |
-      // |--------------------------| --     |
-      // | Padding after RVV        | |      |
-      // | (not counted in          | |      |
-      // | MFI.getStackSize())      | |      |
-      // |--------------------------| --     |-- MFI.getStackSize()
-      // | RVV objects              | |      |
-      // | (not counted in          | |      |
-      // | MFI.getStackSize())      | |      |
-      // |--------------------------| --     |
-      // | Padding before RVV       | |      |
-      // | (not counted in          | |      |
-      // | MFI.getStackSize())      | |      |
-      // |--------------------------| --     |
-      // | scalar local variables   | | <----'
-      // |--------------------------| -- <-- BP
-      // | VarSize objects          | |
-      // |--------------------------| -- <-- SP
+    // |--------------------------| -- <-- BP (if var sized objects present)
+    // 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.
+    if (hasBP(MF)) {
+      FrameReg = RISCVABI::getBPReg();
     } else {
+      // VarSize objects must be empty in this case!
+      assert(!MFI.hasVarSizedObjects());
       FrameReg = RISCV::X2;
-      // |--------------------------| -- <-- FP
-      // | callee-allocated save    | | <----|
-      // | area for register varargs| |      |
-      // |--------------------------| |      |
-      // | callee-saved registers   | |      |
-      // |--------------------------| --     |
-      // | realignment (the size of | |      |
-      // | this area is not counted | |      |
-      // | in MFI.getStackSize())   | |      |
-      // |--------------------------| --     |
-      // | Padding after RVV        | |      |
-      // | (not counted in          | |      |
-      // | MFI.getStackSize())      | |      |
-      // |--------------------------| --     |-- MFI.getStackSize()
-      // | RVV objects              | |      |
-      // | (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 8 bytes.
     if (MFI.getStackID(FI) == TargetStackID::Default) {
       Offset += StackOffset::getFixed(MFI.getStackSize());
       if (FI < 0)
         Offset += StackOffset::getFixed(RVFI->getLibCallStackSize());
     } else if (MFI.getStackID(FI) == TargetStackID::ScalableVector) {
       Offset += StackOffset::get(
           alignTo(MFI.getStackSize() - RVFI->getCalleeSavedStackSize(), 8),
           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| |
       // |--------------------------| |