[PATCH] D94465: [RISCV] Frame handling for RISC-V V extension. (2nd. version)

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:394
   // Early exit if there is no need to allocate on the stack
-  if (RealStackSize == 0 && !MFI.adjustsStack())
+  if (RealStackSize == 0 && !MFI.adjustsStack() && !RVVStackSize)
     return;
----------------
nit: It's a bit odd to have one `== 0` comparison and one `!` comparison in the same line. I'd favour consistency here.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:624
+    Offset = StackOffset::getScalable(MFI.getObjectOffset(FI));
+  }
 
----------------
`else ...`? I think we should probably assert here.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:780
+  // Allocate all RVV locals and spills
+  for (unsigned FI : ObjectsToAllocate) {
+    // ObjectSize in bytes.
----------------
This is implicitly converting from `int` to `unsigned`. Is using `int` okay?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:154
 
+Register RISCVRegisterInfo::getVLENFactoredAmount(
+    MachineFunction &MF, MachineBasicBlock &MBB, MachineBasicBlock::iterator II,
----------------
This parameter named `Amount` implies the function can be used generically, but the variables and assertions within specifically imply the amount is taken to be "the number of registers". If this function is truly generic I think things like `NumOfVReg` should be renamed, but if the function makes assumptions about what `Amount` is then that should be clarified.

Also, if the function is generic maybe it should live in `RISCVInstrInfo`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94465