[PATCH] D81267: [LV] Enable the LoopVectorizer to create pointer inductions

[Anna Welker via Phabricator via llvm-commits]

anwel added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4222
     }
-    return;
   }
----------------
SjoerdMeijer wrote:
> SjoerdMeijer wrote:
> > nit: unnecessary change?
> Nit: can we just return here, and get rid of the "else"? That saves some indentation.
Yes, but also unnecessary return in the first place, and a change very close to the relevant changes?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4222
+      }
+    } else {
+      assert(isa<SCEVConstant>(II.getStep()) &&
----------------
anwel wrote:
> SjoerdMeijer wrote:
> > SjoerdMeijer wrote:
> > > nit: unnecessary change?
> > Nit: can we just return here, and get rid of the "else"? That saves some indentation.
> Yes, but also unnecessary return in the first place, and a change very close to the relevant changes?
Yes, we can.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4223
+    } else {
+      assert(isa<SCEVConstant>(II.getStep()) &&
+             "Induction step not a SCEV constant!");
----------------
SjoerdMeijer wrote:
> Nit: I don't know if the step is always a constant for a PtrInduction, so don't know if this should an assert or just a return.
Using the assert here was inspired by emitTransformedIndex, which was - and, if Cost->isScalarAfterVectorization(P, VF) is true, still is - used for scalarising the induction. It uses the exact same assert for the step of a pointer induction.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4230
+      Type *ScStValueType = ScalarStartValue->getType();
+      PHINode *NewOldPhi =
+          PHINode::Create(ScStValueType, 2, "pointer.phi", Induction);
----------------
SjoerdMeijer wrote:
> Nit: NewOldPhi is a bit of a confusing name.
Okay, maybe a bit confusing. I renamed it to NewPointerPhi, that's a bit more straightforward.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4246
+
+      // Create UF many actual address geps
+      for (unsigned Part = 0; Part < UF; ++Part) {
----------------
SjoerdMeijer wrote:
> nit: this comment could be more descriptive.
Comment has been made more descriptive :)


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

https://reviews.llvm.org/D81267