[PATCH] D117140: [LV] Always create VPWidenCanonicalIVRecipe, optimize away later.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.
Herald added a subscriber: alextsao1999.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9210
 
+  VPlanTransforms::removeRedundantVPWidenCanonicalIVRecipe(*Plan);
   VPlanTransforms::removeRedundantInductionCasts(*Plan);
----------------
Suffice to call it removeRedundantCanonicalIVs ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1256
 
+bool VPWidenIntOrFpInductionRecipe::isCanonical() const {
+  auto *StartC = dyn_cast<ConstantInt>(getStartValue()->getLiveInIRValue());
----------------
Original thought was to leverage
 static bool isCanonicalID(const InductionDescriptor &ID)

Can potentially specialize upon construction and have a recipe which needs no Start nor Step operands, as these are inherently known constants for such IV's?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1071
+  /// Returns the type of the original IV.
+  const Type *getOriginalType() const { return IV->getType(); }
 };
----------------
It's also the scalar/element type of the new IV; how about getScalarType?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:342
+  VPWidenIntOrFpInductionRecipe *WidenOriginalIV = nullptr;
+  for (VPRecipeBase &Phi : *HeaderVPBB) {
+    WidenOriginalIV = dyn_cast<VPWidenIntOrFpInductionRecipe>(&Phi);
----------------
Suffice to traverse HeaderVPBB->phis()?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:347
+
+    // If the induction recipe is for the primary induction use it directly.
+    if (WidenOriginalIV->isCanonical() &&
----------------
Comment should be updated.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:349
+    if (WidenOriginalIV->isCanonical() &&
+        WidenOriginalIV->getOriginalType() == CanonicalIV->getOriginalType())
+      break;
----------------
Would slightly be better to compare with WidenNewIV->getOriginalType()/getScalarType()?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:50
+  /// Try to replace VPWidenCanonicalIVRecipes with the primary IV recipe, if it
+  /// exists.
+  static void removeRedundantVPWidenCanonicalIVRecipe(VPlan &Plan);
----------------
Comment should be updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117140