[PATCH] D106857: [RISCV] Teach VSETVLI insertion to merge the unused VSETVLI with the one need to be insert after it.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:636
   VSETVLIInfo CurInfo;
+  MachineInstr *VSetMI = nullptr;
 
----------------
jacquesguan wrote:
> frasercrmck wrote:
> > Could this be a property of `VSETVLIInfo` somehow? It seems odd to track two separate bits of state.
> > 
> > If not I think the variable needs a better name.
> Done, thanks.
Putting it in VSETVLIINfo allocates space for every basic block as part of the global analysis. But it is only used in the function that runs after the global analysis is done. From that perspective I'm not sure it makes sense to be in the class.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:695
+               (NewInfo.hasAVLReg() &&
+                NewInfo.getAVLReg() == VSETVLIMI->getOperand(0).getReg())))
+            VSETVLIMI->getOperand(2).setImm(NewInfo.encodeVTYPE());
----------------
What if the original vsetvli had a smaller vlmax than the instruction we're inserting for. Won't we lose the clipping that should have existed?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vsetvli-insert.ll:12
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    vsetvli a0, a0, e64, m1, ta, mu
+; CHECK-NEXT:    vfadd.vv v8, v8, v9
----------------
What if a0 is used by another instruction or a pointer increment? This changes VLMAX which changes where AVL gets clipped, potentially breaking the other uses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106857