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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 9 inline comments as done.
fhahn added inline comments.


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


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

I think it would make sense to have `VPWidenIntOrFpInductionRecipe::isCanonical()` replace the existing use of `isCanonicalID`. I put up D117551.

> 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?

That would be an option, but I am not sure if the benefits are worth introducing another IV recipe, as we already have a number of them and checking whether it is a canonical induction is cheap & not needed in many places.


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


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


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


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


================
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);
----------------
Ayal wrote:
> Comment should be updated.
Updated to reference canonical IV.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117140