[PATCH] D129735: [RISCV] Add new pass to transform undef to pseudo for vector values.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:171
+static std::vector<MachineInstr *>
+filterAffectSubRegInst(std::vector<MachineInstr *> Insts) {
+  std::vector<MachineInstr *> rst;
----------------
Why passing `Insts` by value? That will make a copy but it doesn't look like we need a copy.


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:181
+
+static MachineOperand *getLastRegUser(std::vector<MachineInstr *> Insts,
+                                      unsigned Reg) {
----------------
Why passing `Insts` by value? That will make a copy but it doesn't look like we need a copy.


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:182
+static MachineOperand *getLastRegUser(std::vector<MachineInstr *> Insts,
+                                      unsigned Reg) {
+  for (size_t i = Insts.size(); i != 0; i--) {
----------------
unsigned -> Register


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:196
+
+static unsigned getRegNeedToUpdate(std::vector<MachineInstr *> Insts) {
+  MachineInstr *FinalRegProvider = Insts[Insts.size() - 2];
----------------
unsigned -> Register


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:196
+
+static unsigned getRegNeedToUpdate(std::vector<MachineInstr *> Insts) {
+  MachineInstr *FinalRegProvider = Insts[Insts.size() - 2];
----------------
craig.topper wrote:
> unsigned -> Register
Why passing `Insts` by value? That will make a copy but it doesn't look like we need a copy.


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:199
+  assert(FinalRegProvider->getOperand(0).isReg() && "Must be register");
+  unsigned Reg = FinalRegProvider->getOperand(0).getReg();
+  return Reg;
----------------
unsigned -> Register


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:298
+    MachineInstr *ECInst = Candidate.back();
+    unsigned OriginLastestReg = getRegNeedToUpdate(Candidate);
+    unsigned LastestReg = OriginLastestReg;
----------------
unsigned -> Register


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:299
+    unsigned OriginLastestReg = getRegNeedToUpdate(Candidate);
+    unsigned LastestReg = OriginLastestReg;
+    // PseudoDef the sub-register until whole target register is fully defined.
----------------
unsigned -> Register


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:312
+          getVRLargestSuperClass(TRI->getSubRegisterClass(TargetRegClass, ind));
+      unsigned TmpInitSubReg = MRI->createVirtualRegister(SubRegClass);
+      BuildMI(*ECInst->getParent(), ECInst, ECInst->getDebugLoc(),
----------------
createVirtualRegister returns `Register` not `unsigned`


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:316
+              TmpInitSubReg);
+      unsigned NewReg = MRI->createVirtualRegister(TargetRegClass);
+      BuildMI(*ECInst->getParent(), ECInst, ECInst->getDebugLoc(),
----------------
createVirtualRegister returns `Register` not `unsigned`


================
Comment at: llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll:28
 ; CHECK-NEXT:    vsetvli zero, a0, e16, m4, ta, ma
-; CHECK-NEXT:    vloxseg2ei32.v v8, (a0), v8
+; CHECK-NEXT:    vloxseg2ei32.v v16, (a0), v8
 ; CHECK-NEXT:    csrr a0, vlenb
----------------
BeMg wrote:
> craig.topper wrote:
> > Are we treating insert_subreg for segment load tuples the same as inserting a small LMUL into a wider LMUL?
> This pass doesn't consider segment load as instruction that assign sub-register. 
> 
> The following Insert_subreg work like put %5:vrm2 into %6:vrm4
> ```
> %1:vrm4 = IMPLICIT_DEF
> %5:vrm2 = PseudoVLE32_V_M2 killed %4, 0, 5 /* e32 */
> %6:vrm4 = INSERT_SUBREG %1, %5, %subreg.sub_vrm2_0
> ```
> 
> Do we should treat vloxseg2ei32 as INSERT_SUBREG in this patch?
Nevermind, I didn't realize this test had so many `undef` and `poison` operands. I suspect llvm-reduce or bugpoint. I dislike tests with undef/poison operands. It makes things very fragile. It would be legal for DAG combine to delete a large portion of this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129735