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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.
Herald added subscribers: sunshaoce, pcwang-thead, eopXD, VincentWu, arichardson.
Herald added a project: All.


================
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:
----------------
jrtc27 wrote:
> 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).
This is missing LBU, LHU, and LWU.

I agree with @jrtc27 we should check the I or S type instead.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:323
+  const RISCVFrameLowering *TFI = getFrameLowering(MF);
+  unsigned int SizeOfGPRInByte = getRegSizeInBits(RISCV::GPRRegClass) / 8;
+  Offset += getFrameIndexInstrOffset(MI, FIIndex);
----------------
No need to say 'int' llvm almost always uses `unsigned` by itself.


================
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.
----------------
jrtc27 wrote:
> 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)?
Are X3 and X4 really callee saved to the stack? They're reserved so I don't think we ever spill them.


================
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))
----------------
asb wrote:
> 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?
I think vector load/store no longer have frame indices.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98101