[PATCH] D101076: [SVE][LoopVectorize] Add support for scalable vectorization of first-order recurrences

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @kmclaughlin, the latest patch looks a lot neater! I've not finished reviewing all the tests yet, but just a few minor comments so far ...



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4144
+    auto *RuntimeVF = getRuntimeVF(Builder, IdxTy, VF);
+    auto *LastIdx = Builder.CreateBinOp(Instruction::Sub, RuntimeVF, One);
     VectorInit = Builder.CreateInsertElement(
----------------
nit: I think we can use call Builder.GetSub here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4213
+    auto *RuntimeVF = getRuntimeVF(Builder, IdxTy, VF);
+    auto *LastIdx = Builder.CreateBinOp(Instruction::Sub, RuntimeVF, One);
+    ExtractForScalar = Builder.CreateExtractElement(ExtractForScalar, LastIdx,
----------------
nit: I think we can just call Builder.GetSub here instead?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/first-order-recurrence.ll:120
+; CHECK: %[[SUB1:.*]] = sub i32 %[[MUL1]], 1
+; CHECK: %vector.recur.init = insertelement <vscale x 4 x i16> poison, i16 %0, i32 %[[SUB1]]
+; CHECK: vector.body:
----------------
nit: Perhaps better to use a named variables in this test instead of `%vector.recur.init` and `%vector.recur` as they chould change?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/first-order-recurrence.ll:132
+; CHECK: %[[SUB2:.*]] = sub i32 %[[MUL2]], 1
+; CHECK: %vector.recur.extract = extractelement <vscale x 4 x i16> %[[L1]], i32 %[[SUB2]]
+entry:
----------------
nit: Can just use `%{{.*}}` here I think.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/first-order-recurrence.ll:178
+;
+define void @PR26734(i16* %a, i32* %b, i32* %c, i32 %d, i16* %e) {
+; CHECK-LABEL: @PR26734(
----------------
I wonder if we need these negative tests (PR26734, PR27246 and no_sink_after), since they're already covered for fixed width vectors in llvm/test/Transforms/LoopVectorize/first-order-recurrence.ll and in this patch you've only changed fixRecurrence, which only gets invoked when deciding to vectorise?


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

https://reviews.llvm.org/D101076