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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
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 = [&]() {
----------------
fhahn wrote:
> The comment seems a bit out of place now, should it be around line 1910?
+1
Some explanation what this lambda is for may be good here though. It basically creates the vector values from the scalar IV, in the absence of creating a vector IV.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1816
   // required to be loop-invariant
-  assert(PSE.getSE()->isLoopInvariant(ID.getStep(), OrigLoop) &&
-         "Induction step should be loop invariant");
-  auto &DL = OrigLoop->getHeader()->getModule()->getDataLayout();
-  Value *Step = nullptr;
-  if (PSE.getSE()->isSCEVable(IV->getType())) {
-    SCEVExpander Exp(*PSE.getSE(), DL, "induction");
-    Step = Exp.expandCodeFor(ID.getStep(), ID.getStep()->getType(),
-                             LoopVectorPreHeader->getTerminator());
-  } else {
-    Step = cast<SCEVUnknown>(ID.getStep())->getValue();
-  }
-
-  // Try to create a new independent vector induction variable. If we can't
-  // create the phi node, we will splat the scalar induction variable in each
-  // loop iteration.
-  if (VF > 1 && !shouldScalarizeInstruction(EntryVal)) {
-    createVectorIntOrFpInductionPHI(ID, Step, EntryVal);
-    VectorizedIV = true;
-  }
+  auto CreateStepValue = [&]() -> Value * {
+    assert(PSE.getSE()->isLoopInvariant(ID.getStep(), OrigLoop) &&
----------------
Seems like it may be good to pass in ID.getStep() as an explicit parameter, as it forms the basis of the step being created.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1823
+                               LoopVectorPreHeader->getTerminator());
+    } else
+      return cast<SCEVUnknown>(ID.getStep())->getValue();
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1827
 
-  // 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
-  // values. If we were given a truncation type, truncate the canonical
-  // induction variable and step. Otherwise, derive these values from the
-  // induction descriptor.
-  if (!VectorizedIV || NeedsScalarIV) {
-    ScalarIV = Induction;
+  auto CreateScalarIV = [&](Value *&Step) -> Value * {
+    Value *ScalarIV = Induction;
----------------
+1 about the first part of the original comment "If we haven't yet vectorized the induction variable..." seeming out of place here now.
But the remaining part that explains what this code is doing, would help here to describe what this lambda is for, including that comment about truncation.
Possibly also resurrecting the original comment about ScalarIV (changing "will be" to "is"):
  // The scalar value to broadcast. This will be derived from the canonical
  // induction variable.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1859
+
+  auto CreateScalarIVCode = [&](Value *Step) {
+    Value *ScalarIV = CreateScalarIV(Step);
----------------
Is CreateScalarIVCode() really needed, instead of invoking

```
Value *ScalarIV = CreateScalarIV(Step);
CreateSplatIV(ScalarIV, Step);
```
directly, as done below before calling buildScalarSteps()?
(They are not folded together because CreateScalarIV() may change Step, right?)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1883
+  auto NeedsScalarIV = needsScalarInduction(EntryVal);
+
+  // Try to create a new independent vector induction variable. If we can't
----------------
Can continue early-exiting by doing next

```
if (!NeedsScalarIV) {
  createVectorIntOrFpInductionPHI(ID, Step, EntryVal);
  return;
}
```
cleaning up the checks for NeedsScalarIV below.


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

https://reviews.llvm.org/D76686