[PATCH] D78203: [VP,Integer,#2] ExpandVectorPredication pass

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

Aside from my nitpicks, I was wondering if it's possible to add tests for the other strategies. Even if that involved a strategy that you could force on the command-line, since I know we don't have real TTIs just now. It would be nice to see that the pass behaves in the various ways it's meant to.



================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:312
+
+  // Drop the EVl parameter
+  discardEVLParameter(VPI);
----------------
stray lower-case l in `EVL` (or should it be `%evl`?)


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:317
+
+  // re-asses the modified instruction
+  return &VPI;
----------------
typo (`reassess`), also preferably with a capital `R` to match the style of the other comments


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:351
+void sanitizeStrategy(Instruction &I, VPLegalization &LegalizeStrat) {
+  // Speculatable instructions do not strictle need predication.
+  if (isSafeToSpeculativelyExecute(&I))
----------------
typo: `strictly`


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:364
+bool CachingVPExpander::expandVectorPredication() {
+  // Holds all vector-predicated ops with an effective vector length param that
+  SmallVector<TransformJob, 16> Worklist;
----------------
Comment trails off here.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:386
+      break;
+    case VPLegalization::Discard: {
+      discardEVLParameter(*Job.PI);
----------------
You shouldn't need the parentheses in these switch cases, also the formatting with the `break`s looks a little off to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78203