[PATCH] D147567: [VPlan] Only create extracts for recurrence exits if there are live-outs.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

LGTM, adding minor nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3837
   Value *Incoming = State.get(PreviousDef, UF - 1);
   auto *ExtractForScalar = Incoming;
   auto *IdxTy = Builder.getInt32Ty();
----------------
nit: define ExtractForScalar closer to its use below?

Set insert point, and possibly RuntimeVF, early for both Extract's.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3844
     auto *LastIdx = Builder.CreateSub(RuntimeVF, One);
     ExtractForScalar = Builder.CreateExtractElement(ExtractForScalar, LastIdx,
                                                     "vector.recur.extract");
----------------
nit: use Incoming instead of ExtractForScalar, similar to ExtractForPhiUsedOutsideLoop.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3856
+
   Value *ExtractForPhiUsedOutsideLoop = nullptr;
+  if (!LiveOuts.empty()) {
----------------
nit: define ExtractForPhiUsedOutsideLoop inside the if below?

Names of both ExtractFor's could be better aligned (independent of this patch), perhaps ExtractForPhiInScalarLoop?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3868
+          Incoming, Idx, "vector.recur.extract.for.phi");
+    } else if (UF > 1)
+      // When loop is unrolled without vectorizing, initialize
----------------
nit: should this `else if (UF > 1)` be an `else { assert(UF > 1 && "VF and UF are both 1?"); ...}`?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147567