[PATCH] D126403: [RISCV] reorganize getFrameIndexReference to reduce code duplication [nfc]

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

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



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:758
+      assert(!RI->hasStackRealignment(MF) &&
+             "can't index across variable sized realign");
+      // We don't expect any extra RVV alignment padding, as the stack size
----------------
A quick grep shows that starting assertion messages with lower-case letters is far less common in the RISCV backend than upper-case ones.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:766
+    }
+  } else {
+    // This case handles indexing off both SP and BP.
----------------
What would you say to an early return here after `FrameReg == getFPReg(STI)`?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:794
+    // | scalar local variables   | | <----'
+    // |--------------------------| -- <-- SP
+    //
----------------
StephenFan wrote:
> Do you think it would be better to mention the BP here?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126403/new/

https://reviews.llvm.org/D126403