[PATCH] D89449: [RISCV] Initial infrastructure for code generation of the RISC-V V-extension

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoPseudoV.td:145
+multiclass pseudo_binary_v_vv_vx_vi<Operand imm_type = simm5,
+                                    bit force_earlyclobber = 0> {
+  let mayLoad = 0, mayStore = 0, hasSideEffects = 0, usesCustomInserter = 1 in
----------------
frasercrmck wrote:
> I notice that the RFC mentions using `early-clobber` constraints but don't see it being used here. From the RFC:
> 
> ```
> early-clobber %2:vrm2 = PseudoVADD_VV_M2 %3:vrm2(tied-def 0), %0:vrm2, %1:vrm2,$noreg, $x0, 32, implicit $vl, implicit $vtype
> 
> (If you wonder about the early-clobber it is needed to fulfill some constraints between sources and destination registers under lmul>1)
> ```
> 
> I ask because I'm concerned about the use of `tied` and `early-clobber` on the same operand: it is a special-case in SlotIndexes (as once explained in the [[ http://lists.llvm.org/pipermail/llvm-dev/2017-February/110492.html | mailing lists ]]) and I've seen issues with this on another target I was working on, where LLVM forgets about this special case in several places and generates wrong code (subregister lanes are incorrectly deemed to be `undef`). I worry we're going to see really hard-to-track bugs a few months down the line when trying to compile more complex programs.
> 
> Is `early-clobber` really needed? Perhaps you could explain which constraints under lmul>1 are fulfilled by using this?
Hi, apologies I wasn't clear enough with this aspect of the proposal.

`early-clobber` is in practice only relevant for widenings and narrowings (and a few other instructions). That is the reason why it is not in this very first patch. In the particular case of widenings and narrowings, we cannot have a def operand and a use operand where their `sew` is different and their actual vector registers overlap (under some conditions).

For instance `vwadd.vv v2, v1, v2` is not valid (the rule is a bit obscure as I understand `vwadd.vv v2, v1, v3` might be valid, see https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#52-vector-operands). The simplest way to avoid this issue was using `early-clobber`.

If we don't use `early-clobber` then I understand we need to amend somehow the instructions after RA. Perhaps it is possible to let RA know what registers are still feasible as it goes allocating them? (I have not looked into that, tbh)

There are some cases where `early-clobber` may be too strict. I asked the list whether there is a way to model something more than what `early-clobber` does (for operands that have the same EEW as the destination, consider `vwadd.wv`)  in http://lists.llvm.org/pipermail/llvm-dev/2020-May/141383.html but apparently there is no straightforward solution at the moment.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:94
+  // V registers for code generation. We handle them manually.
+  markSuperRegs(Reserved, RISCV::VL);
+  markSuperRegs(Reserved, RISCV::VTYPE);
----------------
NickHung wrote:
> The reserved VL and VTYPE would immediately dead after implicit def. How do you support the calling convention?  Both CSRs are caller-saved.
To be honest we haven't outlined any calling convention at this stage yet. See for some ideas we're considering but nothing is set in stone https://github.com/riscv/rvv-intrinsic-doc/issues/38

However as you mention, even if both CSRs are caller-saved and given the current mechanism in which every instruction using them is prefixed with a `vsetvl`, `vl` is implicitly saved in a GPR whose value will be preserved through the call.

Until the calling convention is clarified, any pass that removes redundant `vsetvl` instructions must be aware that calls may have clobbered it.

>From your question, though, now I realize that we want to extend the lowering of a call in selectiondag to assert in the regmask that `vl` and `vtype` are clobbered. I think this would the safe thing to do for the usual calling convention. Does this seem reasonable at this stage?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D89449