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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3837
   Value *Incoming = State.get(PreviousDef, UF - 1);
   auto *ExtractForScalar = Incoming;
   auto *IdxTy = Builder.getInt32Ty();
----------------
Ayal wrote:
> nit: define ExtractForScalar closer to its use below?
> 
> Set insert point, and possibly RuntimeVF, early for both Extract's.
Will do separately.


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3856
+
   Value *ExtractForPhiUsedOutsideLoop = nullptr;
+  if (!LiveOuts.empty()) {
----------------
Ayal wrote:
> nit: define ExtractForPhiUsedOutsideLoop inside the if below?
> 
> Names of both ExtractFor's could be better aligned (independent of this patch), perhaps ExtractForPhiInScalarLoop?
Moved inside the loop. I'll address the suggestions independent to the patch as follow-up.


================
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
----------------
Ayal wrote:
> nit: should this `else if (UF > 1)` be an `else { assert(UF > 1 && "VF and UF are both 1?"); ...}`?
> 
Looks like it, I'll turn it into an assert separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147567