[PATCH] D16197: [LV] Vectorize first-order recurrences

[Matthew Simpson via llvm-commits]

mssimpso added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3613-3632
@@ +3612,22 @@
+
+  // First, ensure the phi is in the original loop header.
+  assert(Phi->getParent() == OrigLoop->getHeader() &&
+         "Phi node isn't in loop header");
+
+  // Ensure the phi has two incoming values.
+  assert(Phi->getNumIncomingValues() == 2 &&
+         "Phi node doesn't have 2 incoming values");
+
+  // Get the original loop preheader and single loop latch. We ensured the loop
+  // had these blocks when detecting the recurrence.
+  auto *Preheader = OrigLoop->getLoopPreheader();
+  auto *Latch = OrigLoop->getLoopLatch();
+  assert(Preheader && "Loop doesn't have a preheader");
+  assert(Latch && "Loop doesn't have single latch");
+
+  // Ensure the loop preheader and latch are incoming blocks of the phi.
+  assert(Phi->getBasicBlockIndex(Preheader) >= 0 &&
+         "Preheader is not an incoming block to phi node");
+  assert(Phi->getBasicBlockIndex(Latch) >= 0 &&
+         "Latch is not an incoming block to phi node");
+
----------------
anemet wrote:
> Don't we only enter this function if the Phi has passed isFirstOrderRecurrence?  Why are we asserting the same properties that were already checked there?  This is almost like saying assert(isFirstOrderRecurrence(Phi)), no?  I don't think we want these unless I am misunderstanding something.
Yes, this is my mistake. I thought you had requested these asserts in an earlier review. I've removed the duplication.

================
Comment at: test/Transforms/LoopVectorize/AArch64/first-order-recurrence.ll:18
@@ +17,3 @@
+; CHECK:   %vector.recur = phi <4 x i32> [ %vector.recur.init, %vector.ph ]
+; CHECK:   shufflevector <4 x i32> %vector.recur
+;
----------------
anemet wrote:
> Is there any reason we can't fully spell out most of these instructions, e.g. shuffle mask?
We can definitely do this.


http://reviews.llvm.org/D16197