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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:198
+        .ChangeToRegister(FrameReg, false, false, FrameRegIsKill);
+    if (!((RISCVVPseudosTable::getPseudoInfo(MI.getOpcode()) != nullptr) &&
+          MI.mayLoadOrStore()))
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94465