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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:311
+                                           int64_t Amount) const {
+  assert(Amount != 0 && "There is no need to adjust stack pointer for RVV.");
+
----------------
Currently it reads as an absolute statement regardless of `Amount`.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:556
 
-  const auto &CSI = getNonLibcallCSI(MFI.getCalleeSavedInfo());
+  const auto &CSI = getNonLibcallCSI(MF, MFI.getCalleeSavedInfo());
 
----------------
Caller can do `MF.getFrameInfo().getCalleeSaveInfo()` now, no need to pass it.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:913
+        .addImm(NumOfVReg);
+    BuildMI(MBB, II, DL, TII->get(RISCV::MUL), FactorRegister)
+        .addReg(SizeOfVector)
----------------
Does V require M? V without M would be a stupid combination, but if it's legal we shouldn't assume otherwise.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:221
+
+    if (ScalableValue == 0) {
+      MI.getOperand(FIOperandNum)
----------------
This should never happen given we're in the `else` for `if (!Offset.getScalable())`


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:237
+
+    // Scalable load/store has no immediate field.
+    if (MFI.getStackID(FrameIndex) == TargetStackID::ScalableVector &&
----------------
This step doesn't get a number/summary?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:198
+        .ChangeToRegister(FrameReg, false, false, FrameRegIsKill);
+    if (!((RISCVVPseudosTable::getPseudoInfo(MI.getOpcode()) != nullptr) &&
+          MI.mayLoadOrStore()))
----------------
rogfer01 wrote:
> HsiangKai wrote:
> > rogfer01 wrote:
> > > I'm not sure whether we won't get some cases where the frame index is fixed but the instruction is actually a RVV load/store.
> > > 
> > > Something from IR like below (assume the size of the array is correct for the runtime `vscale` value)
> > > 
> > > ```lang=llvm
> > > %array = alloca [64 x double]
> > > %vptr = bitcast [64 x double]* %array to <vscale x 1 x double>*
> > > %v = load <vscale x 1 x double>, <vscale x 1 x double>* %vptr
> > > ```
> > We may need to add an option to tell the compiler what vector length we are compiling for. Otherwise, how do we confirm the VLS length equals to RVV vector length? Another question is which LMUL do we support to do VLS load/store through RVV instructions, LMUL = 1 only?
> > 
> > I will leave a FIXME here.
> I'm not sure I'm reading the condition right:
> 
> ```lang=cpp
> if (!((RISCVVPseudosTable::getPseudoInfo(MI.getOpcode()) != nullptr) && MI.mayLoadOrStore()))```
> 
> should be the same as
> 
> ```lang=cpp
> if (((RISCVVPseudosTable::getPseudoInfo(MI.getOpcode()) == nullptr) || !MI.mayLoadOrStore()))```
> 
> or slightly simplified
> 
> ```lang=cpp
> if (!RISCVVPseudosTable::getPseudoInfo(MI.getOpcode()) || !MI.mayLoadOrStore()))```
> 
> So my question is: what happens if `!RISCVVPseudosTable::getPseudoInfo(MI.getOpcode())` is `false` (i.e. this is a RVV pseudo) and `!MI.mayLoadOrStore()` is `false` too (say it is `RISCV::PseudoVLE64_V`)?
> 
> I understand the LLVM IR snippet above can generate a case like this one: fixed offset but an RVV pseudo that does loads/store.
> 
> We can address this case in a later change. If we do, I suggest we add a `report_fatal_error` to clearly state this is not supported yet.
> 
> What do you think?
Your simplified form is much easier to parse; I'd prefer the patch use that instead.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/allocate-lmul-2-4-8.ll:10
+; CHECK-NEXT:    sub sp, sp, a0
+; CHECK-NEXT:    csrr a0, vlenb
+; CHECK-NEXT:    add sp, sp, a0
----------------
This should only be read once per function, especially since CSR accesses can be full pipeline flushes on some microarchitectures (though one would hope that something like `vlenb` wouldn't, being a constant CSR, in a high performance chip, but less code is still good). Though you'd still likely want it to be rematerialisable rather than spilled to the stack.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/localvar.ll:123
+; RV64IV-NEXT:    sub sp, sp, a0
+; RV64IV-NEXT:    vsetvli a0, zero, e8,m8,ta,mu
+; RV64IV-NEXT:    csrr a0, vlenb
----------------
Ugh what ugly syntax they chose, should have gone with `|` or something (or at least put the whole thing in parentheses if they really want to use commas).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94465