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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

The GCC torture suite now gets a 100% pass rate for me - thanks for fixing. I've left various minor notes about comment phrasing and formatting etc.

I don't feel I've stepped through the logic of the patch carefully enough yet, but hopefully the suggested edits are actionable in the meantime.



================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:291
 
+// Returns true if the target wants the LocalStackSlotAllocation pass to be run
+// and virtual base regsiters used for more efficient stack access.
----------------
No need to copy this doc comment TargetRegisterInfo.h.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:298
+
+// returns true if the instruction's frame index reference would be better
+// served by a base register other than FP or SP.
----------------
returns => Returns


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:300
+// served by a base register other than FP or SP.
+// Used by LocalStackSlotAllocation to determine which frame index references it
+// should create new base registers for.
----------------
I think this sentence needs rewriting - I can't quite follow it.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:305
+  unsigned FIIndex = 0;
+  for (;!MI->getOperand(FIIndex).isFI(); FIIndex++)
+    assert(FIIndex < MI->getNumOperands() && 
----------------
clang-format prefers this with a space after the first `;`


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:328
+  // Assume all the callee saved registers are modified to estimate the max
+  // possible offset that relative to frame pointer
+  // Callee saved registers: X1, X3, X4, X8-9, X18-27.
----------------
I think: "the maximum possible offset relative to the frame pointer."?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:334
+
+  // Assume 128 bytes spill slots size to estimate the max possible offset that
+  // relative to the stack pointer
----------------
bytes => byte and "maximum possible offset relative to the stack pointer." I think


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:350
+// Insert defining instruction(s) for a pointer to FrameIdx before
+// insertion point I. Return materialized frame pointer
+Register RISCVRegisterInfo::materializeFrameBaseRegister(MachineBasicBlock *MBB,
----------------
Nit: end with full stop


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:369
+
+// Resolve a frame index operand of an instruction
+// to reference the indicated base register plus offset instead
----------------
Nit: re-wrap and end sentence in full stop.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:379
+  // has a FrameIndex operand. These instructions' immediate operand
+  // follow the FrameIndex operand
+  unsigned ImmIndex = FIIndex + 1;
----------------
Nit: end with full stop


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:387
+// Get the offset from the referenced frame index in the instruction,
+// if there is one
+int64_t RISCVRegisterInfo::getFrameIndexInstrOffset(const MachineInstr *MI,
----------------
Nit: end with full stop.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:291
+  // For RISC-V, The machine instructions that include a FrameIndex operand
+  // are load/store(exclude vector load/store), ADDI instructions.
+  if (!MI->mayLoad() && !MI->mayStore() && !(MI->getOpcode() == RISCV::ADDI))
----------------
StephenFan wrote:
> craig.topper wrote:
> > StephenFan wrote:
> > > craig.topper wrote:
> > > > I think vector load/store can have a frameindex operand. There's no immediate field so it will get split out into an ADDI in eliminateFrameIndex, but I think that happens later.
> > > In the RISCVIselDAGToDAG.cpp:
> > > ```
> > > case ISD::FrameIndex: {
> > >     SDValue Imm = CurDAG->getTargetConstant(0, DL, XLenVT);
> > >     int FI = cast<FrameIndexSDNode>(Node)->getIndex();
> > >     SDValue TFI = CurDAG->getTargetFrameIndex(FI, VT);
> > >     ReplaceNode(Node, CurDAG->getMachineNode(RISCV::ADDI, DL, VT, TFI, Imm));
> > >     return;
> > >   }
> > > ```
> > > So the llvm ir like this :
> > > ```
> > > %addr = alloca i8
> > > call void callee(i8* %addr)
> > > ```
> > > the %addr will be selected to ADDI
> > Right. I was just asking why the comment says vector load/store are different?
> Oh, sorry. You're right, the vector load/store can have a FrameIndex Operand. I made a mistake. 
This thread is marked as done, but the comment still same to make the same claim that vector load/store don't have a frameindex?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98101