[PATCH] D98101: [RISCV] Enable the LocalStackSlotAllocation pass support

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:310-314
+    case RISCV::LB: case RISCV::LH: case RISCV::LW: case RISCV::LD:
+    case RISCV::SB: case RISCV::SH: case RISCV::SW: case RISCV::SD:
+    case RISCV::FLH: case RISCV::FLW: case RISCV::FLD:
+    case RISCV::FSH: case RISCV::FSW: case RISCV::FSD:
+    case RISCV::ADDI:
----------------
Can we not just look and see if it both has an FI and is an I or S type instruction? This isn't really flexible, and I don't see why it's needed either (and I'd have to more than double the size of this switch statement downstream in CHERI-LLVM to add both capability base and capability value instructions).


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:325-327
+  // Assume all the callee saved registers are modified to estimate the
+  // maximum possible offset relative to the frame pointer.
+  // Callee saved registers: X1, X3, X4, X8-9, X18-27.
----------------
What about floating point registers? Can we also generalise this to iterate over the list of saved registers rather than copying the ABI to yet another place (and thus yet another place we'd have to change downstream in CHERI-LLVM)?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:332
+
+  // Assume 128 bytes spill slots size to estimate the maximum possible 
+  // offset relative to the stack pointer.
----------------
Why 128? A hard-coded constant common across all ISAs seems fishy.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:372
+                                          int64_t Offset) const {
+  unsigned FIIndex = 0;
+  while (!MI.getOperand(FIIndex).isFI())
----------------
We use the less-confusing name FIOperandNum elsewhere


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:374
+  while (!MI.getOperand(FIIndex).isFI())
+    FIIndex++;
+  Offset += getFrameIndexInstrOffset(&MI, FIIndex);
----------------
The copies of this in RegisterScavenging and other backends have `assert(i < MI.getNumOperands() && "Instr doesn't have FrameIndex operand!");`


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:376-378
+  // For RISC-V, only load/store, ADDI machine instruction
+  // has a FrameIndex operand. These instructions' immediate operand
+  // follow the FrameIndex operand
----------------
I don't see why the first part matters. Only include the relevant information, that "FrameIndex operands are always represented as a register followed by an immediate".


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:379
+  // follow the FrameIndex operand
+  unsigned ImmIndex = FIIndex + 1;
+
----------------
This variable seems pointless to me, just inline the +1


================
Comment at: llvm/test/CodeGen/RISCV/local-stack-allocation.ll:30
 }
\ No newline at end of file

----------------
Please fix this test, either before you pre-commit it or now as another pre-commit if you've already done so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98101