[PATCH] D76686: [LV] widenIntOrFpInduction. NFC.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1816
 
-  // True if we have vectorized the induction variable.
-  auto VectorizedIV = false;
-
-  // Determine if we want a scalar version of the induction variable. This is
-  // true if the induction variable itself is not widened, or if it has at
-  // least one user in the loop that is not widened.
-  auto NeedsScalarIV = VF > 1 && needsScalarInduction(EntryVal);
+  Value *Step = nullptr;
+  auto &DL = OrigLoop->getHeader()->getModule()->getDataLayout();
----------------
Instead of implicitly setting Step in the lambdas, would it be possible to have the closures return the step/IV  they create and take it as argument if required?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1832
 
   // If we haven't yet vectorized the induction variable, or if we will create
   // a scalar one, we need to define the scalar induction variable and step
----------------
Comment seems out of place now.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1858
   // induction variable, and build the necessary step vectors.
   // TODO: Don't do it unless the vectorized IV is really required.
+  auto CreateSplatIV = [&]() {
----------------
The comment seems a bit out of place now, should it be around line 1910?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1859
   // TODO: Don't do it unless the vectorized IV is really required.
-  if (!VectorizedIV) {
+  auto CreateSplatIV = [&]() {
     Value *Broadcasted = getBroadcastInstrs(ScalarIV);
----------------
it might be good to make the VF, Part and Step explicit parameters here (and potentially other places) to make things a bit more explicit at the call sites.


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

https://reviews.llvm.org/D76686