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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

The approach looks very reasonable to me. A first round of mainly nits, while in the mean time I go over this a second time and let things sink in.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4192
 
   // FIXME: The newly created binary instructions should contain nsw/nuw flags,
   // which can be found from the original scalar operations.
----------------
Ah, look at this....I was actually wondering about this last week, and now I found it :)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4222
     }
-    return;
   }
----------------
nit: unnecessary change?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4222
+      }
+    } else {
+      assert(isa<SCEVConstant>(II.getStep()) &&
----------------
SjoerdMeijer wrote:
> nit: unnecessary change?
Nit: can we just return here, and get rid of the "else"? That saves some indentation.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4223
+    } else {
+      assert(isa<SCEVConstant>(II.getStep()) &&
+             "Induction step not a SCEV constant!");
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4230
+      Type *ScStValueType = ScalarStartValue->getType();
+      PHINode *NewOldPhi =
+          PHINode::Create(ScStValueType, 2, "pointer.phi", Induction);
----------------
Nit: NewOldPhi is a bit of a confusing name.


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4535
   // will be a scalar use, and the pointer is only used by memory accesses, we
   // place the pointer in ScalarPtrs. Otherwise, the pointer is placed in
   // PossibleNonScalarPtrs.
----------------
Nit: this comment is slightly out of date, i.e. the `ScalarPtrs` part....


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4539
+    if (isScalarPtrInduction(MemAccess, Ptr)) {
+      Worklist.insert(cast<Instruction>(Ptr));
+      Instruction *Update = cast<Instruction>(
----------------
.... because we are adding things to `Worklist` here.
I was getting confused a bit if this should not be `ScalarsPtrs`, but because it is an induction it makes sense to add it to Worklist?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4574
+  // getelementptr and (pointer) phi instructions used by memory accesses
+  // requiring a scalar use
   //
----------------
Nit: missing `.`


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

https://reviews.llvm.org/D81267