[PATCH] D102739: [RISCV] Enable cross basic block aware vsetvli insertion

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

Mostly just stylistic things from me at this point. I've written a similar pass before so this is surprisingly familiar.

I do also think some new MIR tests to cover this functionality would be good though.



================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:16
+//
+// Phase 2 uses the informatiom from phase 1 to do a data flow analysis to
+// propagate the VL/VTYPE changes through the function. This gives us the
----------------
Wee typo on `informatiom`


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:173
+  }
+  bool operator!=(const VSETVLIInfo &Other) const { return !(*this == Other); }
+
----------------
Super nit but a blank line between these functions is probably recommended.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:191
+    VSETVLIInfo MergeInfo;
+    MergeInfo.setUnknown();
+    return MergeInfo;
----------------
Is there a reason not to make the constructor call `setUnknown`?

I was wondering if a static `VSETVLIInfo::getUnknown`-style function might improve some use cases. Here would just be `return VSETVLIInfo::getUnknown();` for instance.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:467
+    return;
+  BlockInfo[ThisBlock].Pred = InInfo;
+
----------------
For some reason the lack of a blank line above this stands out to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102739