[PATCH] D125337: [riscv] Prefer to use previous VL for scalar move instructions

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.

Also LGTM aside from nits and possible restructuring. Definitely nicer than the other patch.



================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1232
+      // VL > 0. We can discard the user requested AVL and just use the last
+      // one if we can prove it equally zero.  This remove a setvli entirely
+      // if the types match or allows use of cheaper avl preserving variant
----------------
craig.topper wrote:
> "remove" -> "removes"
`setvli` -> `vsetvli`? 


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1233
+      // one if we can prove it equally zero.  This remove a setvli entirely
+      // if the types match or allows use of cheaper avl preserving variant
+      // if VLMAX doesn't change.  If VLMAX might change, we couldn't use
----------------
`or if it allows the use of a cheaper`


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1235
+      // if VLMAX doesn't change.  If VLMAX might change, we couldn't use
+      // the 'setvli x0, x0 vtype" variant, and avoid the transform to
+      // prevent extending live range of an avl register operand.
----------------
`vsetvli` again? maybe a comma after that last `x0` too


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1235
+      // if VLMAX doesn't change.  If VLMAX might change, we couldn't use
+      // the 'setvli x0, x0 vtype" variant, and avoid the transform to
+      // prevent extending live range of an avl register operand.
----------------
frasercrmck wrote:
> `vsetvli` again? maybe a comma after that last `x0` too
`and avoid` -> `so (we) avoid`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1252
+    if (RISCVII::hasSEWOp(TSFlags)) {
+      CurInfo = computeInfoForInstr(MI, TSFlags, MRI);
+      continue;
----------------
If we don't update the scalar move instruction this is falling through and computing the same as `NewInfo`. I dunno if you think it'd be clearer if we were explicit about that in the previous block (e.g. update `CurInfo` and `continue` in an `else`?)

You could even early-continue if `MI` has a SEW op and it //isn't// a scalar move.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1259
+    if (MI.isCall() || MI.isInlineAsm() || MI.modifiesRegister(RISCV::VL) ||
+        MI.modifiesRegister(RISCV::VTYPE) ) {
+      CurInfo = VSETVLIInfo::getUnknown();
----------------
Do we need parens on this `if`?


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

https://reviews.llvm.org/D125337