[PATCH] D33058: [LV] Sink casts to unravel first order recurrence

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

In general I wonder if this is really the best place to do this. It would be nice if the loop was canonicalised to be in this form given how cheap it is to do. Perhaps LoopSimplify? Not blocking this change, but something to think about.



================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:556
+  if (!Previous || !TheLoop->contains(Previous) || isa<PHINode>(Previous) ||
+      SinkAfter.count(Previous)) // Cannot rely on dominance due to motion.
     return false;
----------------
In what situation can this occur? Isn't this an output vector?


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:564
+    auto *I = Phi->user_back();
+    if (I->isCast() && (I->getParent() == Phi->getParent()) && I->hasOneUse() &
+        DT->dominates(Previous, I->user_back())) {
----------------
Bug here, bitwise `&` used. Which is also telling me that there aren't sufficient tests for this, especially the case where Previous doesn't dominate the cast's user and so the cast can't be legally sunk past Previous.


================
Comment at: test/Transforms/LoopVectorize/first-order-recurrence.ll:401
+; SINK-AFTER-LABEL: sink_after
+; SINK-AFTER: vector.body
+; void sink_after(short *a, int n, int *b) {
----------------
Can we add a few more checks to verify the result of the vectorization is sane?


https://reviews.llvm.org/D33058