[RISCV] reorganize comments in getFrameIndexReference [nfc]

[reames]

Summary

Follow up to D126403. Reduce comment duplication, and give a high level description of what the code is doing.

For specific review attention, notice that the comment blocks being removed do not agree. In particular, they differ in the order of RVV Objects and scalar local variables. I believe the comment in the FP register path to have simply been wrong, do others agree?

[StephenFan]

LGTM

[frasercrmck]

No, I think the FP path is indeed different. The offsetting code lines up with the comment, doesn't it? That's what I gathered when working on this recently, anyway - I don't have the ability to verify it right now. I don't think it's correct to remove that comment wholesale. The only thing coming to me right now is a comment somewhere (code, commit - sorry) saying we want to put RVV objects as far away from the frame register as possible, hence the scalar/RVV swap when using FP. Or would that be better worded as "scalar as close to the frame register as possible"?

I do think we might be lacking RVV+FP testing. That was something I was going to look into.

[reames]

In D126488#3544664, @frasercrmck wrote:

No, I think the FP path is indeed different. The offsetting code lines up with the comment, doesn't it? That's what I gathered when working on this recently, anyway - I don't have the ability to verify it right now. I don't think it's correct to remove that comment wholesale. The only thing coming to me right now is a comment somewhere (code, commit - sorry) saying we want to put RVV objects as far away from the frame register as possible, hence the scalar/RVV swap when using FP. Or would that be better worded as "scalar as close to the frame register as possible"?

Thank you! Taking another much closer look here, I think you are correct. We do appear to actually change the relative order of the regions of the stack based on whether we have FP or not. That is "surprising" to say the least!

Thinking about it, I think this even vaguely makes sense. By doing it this way, we remove the need to include the variable-but-known size of the RVV region in the offset calculations for local objects.

I do worry about the implications for tooling of having the change in stack layout. Anyone know of anything that might break/complicate unpleasantly?

I do think we might be lacking RVV+FP testing. That was something I was going to look into.

I suspect this as well.

Will adjust this patch to incorporate the changed model.

[reames]

Closing review I'm not currently working on, and am unlikely to get back to in near future. Will reopen if priorities change.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

if (FI >= MinCSFI && FI <= MaxCSFI) {
 
     if (FirstSPAdjustAmount)
       Offset += StackOffset::getFixed(FirstSPAdjustAmount);
     else
       Offset += StackOffset::getFixed(getStackSizeWithRVVPadding(MF));
     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   | |      |
-    // |--------------------------| --     |
-    // | realignment (the size of | |      |
-    // | this area is not counted | |      |
-    // | in MFI.getStackSize())   | |      |
-    // |--------------------------| --     |-- MFI.getStackSize()
-    // | RVV alignment padding    | |      |
-    // | (not counted in          | |      |
-    // | MFI.getStackSize() but   | |      |
-    // | counted in               | |      |
-    // | RVFI.getRVVStackSize())  | |      |
-    // |--------------------------| --     |
-    // | RVV objects              | |      |
-    // | (not counted in          | |      |
-    // | MFI.getStackSize())      | |      |
-    // |--------------------------| --     |
-    // | padding before RVV       | |      |
-    // | (not counted in          | |      |
-    // | MFI.getStackSize() or in | |      |
-    // | RVFI.getRVVStackSize())  | |      |
-    // |--------------------------| --     |
-    // | scalar local variables   | | <----'
-    // |--------------------------| -- <-- BP (if var sized objects present)
-    // | VarSize objects          | |
-    // |--------------------------| -- <-- SP
+
+  // From here, we first select a legal base register to index off, and then
+  // compute the offset expression.  The index expression can't cross a unknown
+  // variable sized region.  We have at two most such regions: 1) dynamic stack
+  // realignment, and 2) variable sized objects.  Both are optional, and may or
+  // may not exist.  Scalable vectors are variable, but not unknown.  We can
+  // compute offset expressions which cross the RVV region.
+  //
+  // Here's what the stack layout looks like:
+  // |--------------------------| -- <-- FP (if present)
+
+  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();
     } else {
       // VarSize objects must be empty in this case!
       assert(!MFI.hasVarSizedObjects());
       FrameReg = RISCV::X2;
     }
   } else {
     FrameReg = RI->getFrameRegister(MF);
   }
 
   if (FrameReg == getFPReg(STI)) {
     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) {
       assert(!RI->hasStackRealignment(MF) &&
              "Can't index across variable sized realign");
       // 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());
     }
     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());
 
   // 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   | | <----'
-  // |--------------------------| -- <-- BP (if var sized objects present)
-  // | VarSize objects          | |
-  // |--------------------------| -- <-- 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)) {
       assert(!RI->hasStackRealignment(MF) &&
              "Can't index across variable sized realign");
       Offset += StackOffset::get(getStackSizeWithRVVPadding(MF) +