[PATCH] D79976: [LV] Handle Fold-Tail of loops with vectorizarion factor (VF) equal to 1

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks for taking care of this, only a couple of nits.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:444
                                    "trip.count.minus.1");
-    Value *VTCMO = Builder.CreateVectorSplat(State->VF, TCMO, "broadcast");
+    Value *VTCMO = State->VF == 1 ? TCMO
+                                  : Builder.CreateVectorSplat(State->VF, TCMO,
----------------
nit: using `auto VF = State->VF` will help shorten this to 2 lines.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:820
     SmallVector<Constant *, 8> Indices;
     for (unsigned Lane = 0, VF = State.VF; Lane < VF; ++Lane)
       Indices.push_back(ConstantInt::get(STy, Part * VF + Lane));
----------------
nit: can move `VF = State.VF` to be reused above and below.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:822
       Indices.push_back(ConstantInt::get(STy, Part * VF + Lane));
-    Constant *VStep = ConstantVector::get(Indices);
+    Constant *VStep = State.VF == 1 ? ConstantInt::get(STy, Part)
+                                    : ConstantVector::get(Indices);
----------------
nit: `ConstantInt::get(STy, Part)` was already pushed inside Indices, can retrieve it e.g. via `Indices.back()`.


================
Comment at: llvm/test/Transforms/LoopVectorize/tail-folding-vectorization-factor-1-scalar.ll:3
+; RUN: opt < %s  -loop-vectorize -force-vector-interleave=4 -S | FileCheck %s
+
+define void @foo() {
----------------
Would be good to explain what this test is for; e.g., that fold-tail produces correct scalar code when LV is only unrolling w/o vectorizing.


================
Comment at: llvm/test/Transforms/LoopVectorize/tail-folding-vectorization-factor-1-vector.ll:3
+; RUN: opt < %s  -loop-vectorize -force-vector-interleave=4 -S | FileCheck %s
+
+define void @foo() !prof !12 {
----------------
Better have both tests in the same file, briefly explain what this tests - possibly using an informative function name rather than "foo".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79976