[PATCH] D126088: [RISCV] Add clarifying asserts to getFrameIndexReference [NFC]

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:693
+    // | scalar local variables   | | <----'
+    // |--------------------------| -- <-- BP (if present)
+    // | VarSize objects          | |
----------------
StephenFan wrote:
> Does `if present` means if the BP exists? But If both realignment and VarSize objects exist, then `BP` must be present.
Please see the code immediately below this.  If you have a suggestion on the comment wording, feel free to suggest something.  


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:718
     if (hasFP(MF)) {
+      assert(FrameReg == getFPReg(STI) && "Frame register not FP");
       Offset += StackOffset::getFixed(RVFI->getVarArgsSaveSize());
----------------
StephenFan wrote:
> 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. 
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.  


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

https://reviews.llvm.org/D126088