[PATCH] D124869: [WIP][RISCV] Hoist VSETVLI out of idiomatic fixed length vector loops

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a reviewer: frasercrmck.
frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:315
 
+  void dump() {
+    dbgs() << (isValid() ? "valid" : "invalid") << ", "
----------------
This is very helpful, thank you!


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1191
+/// we're about to insert by looking for cases where we can PRE from the
+/// begining of one block to the end of one of it's predecessors.  Specifically,
+/// this is geared to catch the common case of a fixed length vsetvl in a single
----------------
craig.topper wrote:
> begining -> beginning
it's -> its


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1202
+    if (BlockInfo[P->getNumber()].Exit.isUnknown()) {
+      if (UnavailablePred) return;
+      UnavailablePred = P;
----------------
`return` on new line


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vector-strided-load-store.ll:520
 ; CHECK-ASM-NEXT:    li a3, 32
+; CHECK-ASM-NEXT:    vsetivli zero, 8, e32, m1, ta, mu
 ; CHECK-ASM-NEXT:    li a4, 16
----------------
This seems like a regression too


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll:521
 ; CHECK-NEXT:    vle16.v v10, (a1)
+; CHECK-NEXT:    vsetvli zero, zero, e16, mf2, ta, mu
 ; CHECK-NEXT:    vwcvt.x.x.v v8, v10
----------------
I'm curious, would this be showing a bug if it was instead `e16,mf2` followed by `vle16.v` in `bb.0` and `e32,m1` followed by `vle32.v` in `bb.1`? If we moved the vsetvli down past the `vle32.v` we'd be using the old vtype with a 2-byte alignment requirement, which the `vle32.v` isn't obliged to use.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124869