[PATCH] D38948: [LV] Support efficient vectorization of an induction with redundant casts

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: ../llvm/include/llvm/Transforms/Utils/LoopUtils.h:311
+  /// associated with the phi includes casts (that we know we can ignore
+  /// under proper runtime checks), they are passed through \Casts.
   static bool isInductionPHI(PHINode *Phi, const Loop* L, ScalarEvolution *SE,
----------------
"\Casts" >> "\p Casts". Or better yet "\p CastsToIgnore"?


================
Comment at: ../llvm/include/llvm/Transforms/Utils/LoopUtils.h:359
+  /// SCEV overflow check.
+  const SmallVector<Instruction *, 2> &getCastInsts() const { 
+    return CastInsts; 
----------------
Better have "const SmallVectorImpl<Instruction *>" as the return type?


================
Comment at: ../llvm/include/llvm/Transforms/Utils/LoopUtils.h:367
+                      BinaryOperator *InductionBinOp = nullptr,
+                      SmallVectorImpl<Instruction *> *CI = nullptr);
 
----------------
Use "Casts" instead of "CI" for consistency.


================
Comment at: ../llvm/include/llvm/Transforms/Utils/LoopUtils.h:379
+  // that are redundant when guarded with a runtime SCEV overflow check.
+  SmallVector<Instruction *, 2> CastInsts;
+
----------------
"CastInsts" >> "RedundantCasts"?


================
Comment at: ../llvm/lib/Analysis/ScalarEvolution.cpp:4389
+///    %CastedPhi = and i64 %PN, 2^n-1
+/// If found, return true, and insert the intermediate casts in \p CastInsts.
+/// Otherwise, return false.
----------------
"casts" >> "cast", or "and".


================
Comment at: ../llvm/lib/Analysis/ScalarEvolution.cpp:4405
+  auto MaskValPlusOne = ++MaskVal;
+  if (!MaskValPlusOne.isPowerOf2())
+    return false;
----------------
Can you check `if (!Mask || !Mask.isMask())` instead?


================
Comment at: ../llvm/lib/Analysis/ScalarEvolution.cpp:4424
+///    %x = phi i64 [ 0, %ph ], [ %add, %for.body ]
+///    %casted_phi = And i64 255
+///    %add = add i64 %casted_phi, %step
----------------
Forgot `%x` to And with 255?


================
Comment at: ../llvm/lib/Analysis/ScalarEvolution.cpp:4428
+/// b) %tmp = shl i64 %x, m 
+///    %casted_phi = ashr exact i64 %sext, m 
+/// e.g.:
----------------
"%sext, m" >> "%tmp, m"?


https://reviews.llvm.org/D38948