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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

I like the idea. We should move vsetvli out when possible. It doesn't look like all the test changes are improvements.



================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1175
+
+/// Return true if the VL value confiuured must be equal to the requested one.
+static bool hasFixedResult(VSETVLIInfo Info, const RISCVSubtarget &ST) {
----------------
confiuuered -> configured


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1186
+  unsigned AVLInBits = AVL * SEW;
+  return ST.getMinRVVVectorSizeInBits() >= AVLInBits;
+}
----------------
You can use the not well named ST.getRealMinVLen() here I think. It takes into account the Zvl*b provided value.


================
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
----------------
begining -> beginning


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1204
+      UnavailablePred = P;
+    } else if (!OtherPred)
+      OtherPred = P;
----------------
Use curly braches for this else body for consistency


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1247
   bool HaveVectorOp = false;
+  DenseSet<const MachineBasicBlock *> BlocksWithVectorOp;
 
----------------
Would it make sense to use a BitVector using MBB numbering?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vsetvli-insert.ll:113
+; CHECK-NEXT:    add a6, a0, a5
+; CHECK-NEXT:    vsetvli zero, a3, e32, m1, ta, mu
+; CHECK-NEXT:    vle32.v v8, (a6)
----------------
This seems worse


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124869