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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Looks good to me, thanks! With some minor nits.



================
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())
----------------
typo:


================
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:
> typo:



================
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();
----------------
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.


================
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.
----------------
The start value is no longer "of ID". Comment worth dropping?


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



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



================
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.
----------------
ID is no longer present, update comment?


================
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();
----------------
nit: check of Kind can (be the first to) early-exit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147891