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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

This looks good to me, but please wait for Silviu to approve as well before committing.



================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1018
+  bool havePredicatedSCEVRewriteForPHI(const SCEVUnknown *PhiScev,
+                                      const SCEVAddRecExpr *AR);
+
----------------
clang-format?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6999
+        continue;
+
       VectorizationCostTy C = getInstructionCost(&I, VF);
----------------
dorit wrote:
> dorit wrote:
> > Ayal wrote:
> > > 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.
> > > 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?
> > 
> > No, because then we would also ignore the cost of the casts when we calculate the baseline cost (of the scalar loop), and we don't want to do that (that loop will not be guarded by the predicate, and the casts will remain there).
> > 
> > > 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.
> > 
> > Yes. I thought it's a small enough fix to be included in this patch. I can submit a separate patch for this. 
> > 
> > Yes. I thought it's a small enough fix to be included in this patch. I can submit a separate patch for this.
> 
> This is https://reviews.llvm.org/D40883.
> 
> thanks,
> Dorit
Great, thanks.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2671
+  const SmallVectorImpl<Instruction *> &Casts = ID.getCastInsts();
+  if (!Casts.empty()) {
+    // Only the first Cast instruction in the Casts vector is of interest.
----------------
Early exit instead if (Casts.empty())?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5873
+
+  return InductionCastsToIgnore.count(Inst);
+}
----------------
Can be folded into a single `return (Inst && InductionCastsToIgnore.count(Inst));`


https://reviews.llvm.org/D38948