[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: lib/Transforms/Utils/LoopUtils.cpp:933
+    D = InductionDescriptor(StartValue, IK_IntInduction, Step, nullptr, 
+                            CastsToIgnore);
     return true;
----------------
Suggest to keep the 'nullptr' parameter as the last argument and continue to use its default, or document /* what-this-nullptr-is */


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:881
+  // Record any Cast instructions that participate in the induction update
+  SmallVector<Instruction *, 2> *CastInsts = nullptr;
+  const auto *SymbolicPhi = dyn_cast<SCEVUnknown>(PhiScev);
----------------
Set CastInsts to nullptr or the address of a SmallVector created earlier depending on the conditions below, or invoke isInductionPHI() once using the default nullptr for its last parameter, or else with such an address.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6999
+        continue;
+
       VectorizationCostTy C = getInstructionCost(&I, VF);
----------------
VecValuesToIgnore holds Instructions whose cost should be ignored only if widened, and for VF's where they are actually widened, IIUC their use in calculateRegisterUsage(). Should the redundant IV casts be added to ValuesToIgnore instead, being redundant in either scalar and vector types?

In any case, having expectedCost() neglect the cost of VecValuesToIgnore may affect cases unrelated to this patch's casted Inductions (related to Reductions), so probably better done in a separate patch.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:619
+                                              Value *VectorLoopValue, 
+                                              unsigned Lane = UINT_MAX);
+
----------------
Placing 'Part' next to 'Lane' seems more logical.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1592
+  /// ignored when creating the vectorized loop body.
+  bool isCastedInductionVariable(Value *V);
+
----------------
The name "isCastedInductionVariable()" may be confused with "isInductionVariable()" - the latter deals with a header Phi, whereas the former deals with a redundant Cast. (The Phi feeding an isCastedInductionVariable() answers true to isInductionVariable(), right?)

Perhaps have "isInductionPhi()", and change "isInductionVariable()" to answer true for both Phi and Cast Instructions of an IV?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5273
+  // because any other casts, if exist, are not used outside the cast sequence,
+  // and will therefore will not be queried.
+  const SmallVectorImpl<Instruction *> &Casts = ID.getCastInsts();
----------------
In other words, all casts could be recorded here for ignoring, but suffices to record only the first.


================
Comment at: test/Transforms/LoopVectorize/vect-phiscev-sext-trunc.ll:51
+; VF1: vector.body:
+; VF1-NOT: %[[TEST:[a-zA-Z0-9.]+]] = shl i32
+; VF1: middle.block:            
----------------
TEST is set but not used.


================
Comment at: test/Transforms/LoopVectorize/vect-phiscev-sext-trunc.ll:119
+; VF1: vector.body:
+; VF1-NOT: %[[TEST:[a-zA-Z0-9.]+]] = shl i64
+; VF1: middle.block:
----------------
TEST is set but not used.


https://reviews.llvm.org/D38948