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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:105
+
+  uint8_t getSEW() const { return SEW; }
+  RISCVII::VLMUL getVLMUL() const { return VLMul; }
----------------
Any reason to restrict this uint8_t? Looks like it's a assigned to unsigned where it's used.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1287
+/// Return true if the VL value configured must be equal to the requested one.
+static bool hasFixedResult(VSETVLIInfo Info, const RISCVSubtarget &ST) {
+  if (!Info.hasAVLImm())
----------------
`Info` could be a const reference I think.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1302
+
+/// Perform simple partial redundancy elimination of the VSETVL instructions
+/// we're about to insert by looking for cases where we can PRE from the
----------------
VSETVL -> VSETVLI


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1317
+  for (MachineBasicBlock *P : MBB.predecessors()) {
+    const VSETVLIInfo PredInfo = BlockInfo[P->getNumber()].Exit;
+    if (PredInfo.isUnknown()) {
----------------
Can PredInfo be a reference? Though maybe it's small enough that copies are cheap?


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1363
+  auto OldInfo = BlockInfo[UnavailablePred->getNumber()].Exit;
+  LLVM_DEBUG(dbgs() << "PRE VSETVLI from " << MBB.getName() << " to " << UnavailablePred->getName()
+                    << " with state " << AvailableInfo << "\n");
----------------
This is more than 80 columns


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

https://reviews.llvm.org/D124869