[PATCH] D98629: [RISCV] Spilling for Zvlsseg registers.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:300
+    SubRegIdx = RISCV::sub_vrm4_0;
+  }
+
----------------
Not sure if it's worth asserting that LMUL is otherwise 1, or `else`ing this?


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:304
+    BuildMI(MBB, MBBI, DL, TII->get(Opcode))
+        .addReg(TRI->getSubReg(SrcReg, SubRegIdx + I))
+        .addReg(Base);
----------------
Other places where we've done computations like this have added `static_assert`s to do basic checks that (e.g.) `sub_vrm2_0+1` is what we expect. It might be worth having some of those.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:187
+    IsZvlsseg = false;
+  } else if (RISCV::VRN2M1RegClass.hasSubClassEq(RC)) {
+    Opcode = RISCV::PseudoVSPILL2_M1;
----------------
I'm surprised clang-tidy hasn't suggested removing the parents around these one-liners. Maybe I'm missing something.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:182
 
+static bool isRVVSpillForZvlsseg(unsigned Opcode, unsigned *NF,
+                                 unsigned *LMUL) {
----------------
This all may be personal preference, but something returning `Optional<std::pair<unsigned, unsigned>>` would remove the out parameters, make this neater to call when you only want the "is" aspect of it, help prevent scope leakage in the caller, and simplify this method definition.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:351
+        TII->getVLENFactoredAmount(MF, MBB, II, ScalableValue);
+    MI.getOperand(FIOperandNum + 1).ChangeToRegister(FactorRegister, false);
+    MI.getOperand(FIOperandNum + 2).ChangeToImmediate(NF);
----------------
`/*IsDef*/`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98629