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

[Adam Nemet via llvm-commits]

anemet 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");
+
----------------
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.

================
Comment at: test/Transforms/LoopVectorize/AArch64/first-order-recurrence.ll:1
@@ +1,2 @@
+; RUN: opt < %s -loop-vectorize -force-vector-interleave=1 -dce -instcombine -S | FileCheck %s
+
----------------
anemet wrote:
> I think that it's better practice to avoid the triple and use -force-vector-width to make the test robust.
Please also add a UF>1 test.

================
Comment at: test/Transforms/LoopVectorize/AArch64/first-order-recurrence.ll:1-4
@@ +1,5 @@
+; RUN: opt < %s -loop-vectorize -force-vector-interleave=1 -dce -instcombine -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64--linux-gnu"
+
----------------
I think that it's better practice to avoid the triple and use -force-vector-width to make the test robust.

================
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
+;
----------------
Is there any reason we can't fully spell out most of these instructions, e.g. shuffle mask?


http://reviews.llvm.org/D16197