[PATCH] D147891: [VPlan] Check VPValue step in isCanonical (NFCI).

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:741
+  // SCEV expansion), but for the canonical induction the step is required to be
+  // 1, which is represented as live-in).
+  if (getStepValue()->getDefiningRecipe())
----------------
Ayal wrote:
> Ayal wrote:
> > typo:
> 
Thanks, adjusted


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:744
+    return false;
+  auto *StepC = dyn_cast<ConstantInt>(getStepValue()->getLiveInIRValue());
   return StartC && StartC->isZero() && StepC && StepC->isOne();
----------------
Ayal wrote:
> nit: setting StartC right after StepC here, instead of above, will (a) early-exit earlier, and (b) highlight how the two are set similarly. Except that Start must be a live-in.
> 
> May be worth having an API that gets the LiveInIRValue if one exists and returns null otherwise, which could then be used by dyn_cast_or_null.
> May be worth having an API that gets the LiveInIRValue if one exists and returns null otherwise, which could then be used by dyn_cast_or_null.

Sounds good, I can see about adding one.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1104
     return false;
   // The start value of ID must match the start value of this canonical
   // induction.
----------------
Ayal wrote:
> The start value is no longer "of ID". Comment worth dropping?
Update comment, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1106
   // induction.
-  if (getStartValue()->getLiveInIRValue() != ID.getStartValue())
+  if (getStartValue() != Start)
+    return false;
----------------
Ayal wrote:
> Ayal wrote:
> > nit: to be consistent with first check of Ty above:
> > 
> 
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1114
+  ConstantInt *StepC = dyn_cast<ConstantInt>(Step->getLiveInIRValue());
   // ID must also be incremented by one. IK_IntInduction always increment the
   // induction by Step, but the binary op may not be set.
----------------
Ayal wrote:
> ID is no longer present, update comment?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1116
   // induction by Step, but the binary op may not be set.
-  return ID.getKind() == InductionDescriptor::IK_IntInduction && Step &&
-         Step->isOne();
+  return Kind == InductionDescriptor::IK_IntInduction && StepC &&
+         StepC->isOne();
----------------
Ayal wrote:
> nit: check of Kind can (be the first to) early-exit.
Moved, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147891