[PATCH] D25632: [LV] Sink scalar operands of predicated instructions

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

That makes sense, especially the cost angle.

On the other hand, my worry is that long-term, as this kind of thing accumulates, we may end up with an ad-hoc "organically grown" mini-optimizer inside the vectorizer. And I'd really like to try to avoid that. The fact this sinking patch requires https://reviews.llvm.org/D25631 to perform DCE inside the vectorizer only reinforces that concern.

So I'm not really sure what we should do. Adam, Gil, thoughts?



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4279
+  // algorithm ends after one pass through the worklist doesn't sink a single
+  // instruction. We index the worklist to avoid iterator invalidations.
+  bool Changed;
----------------
I think the standard way to do this is to run "while (!WorkList.empty()))" and keep a side map of nodes you're done with so you will not add to the worklist. This avoids both the iterator invalidation problem, and recognizing the fixed-point problem.
It may make things more complicated in this case, though.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4291
+          I->getParent() == PredBB) {
+        Worklist.remove(I);
+        continue;
----------------
Is removing from the middle of a SetVector O(N)?
I'd expect this case to happen a lot.

(I understand you're doing it to keep Idx constant while making the Worklist smaller. As above, perhaps change the structure of the loop?)


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4304
+      // remove the instruction from the worklist, and then add its operands.
+      I->moveBefore(PredBB->getFirstNonPHI());
+      Worklist.remove(I);
----------------
PredBB->getFirstInsertionPt() (which is essentially the same thing, but nicer. :-) )


https://reviews.llvm.org/D25632