[PATCH] D99750: [LV, VP]VP intrinsics support for the Loop Vectorizer

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2139
 
+/// A recipe for generating the correct countable exit condition for loops with
+/// the predicated vectorization.
----------------
This is out of date now I think as now the CanonicalIV is left unchanged and used for the countable exit condition. There might be a better name for the recipe, as the current one doesn't seem to capture the essence of the recipe, which effectively represents the current index of elements to process in the current iteration, by account for EVL.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:966
+/// the given idion \p Idiom.
+static void replacePredicateWithIdiom(VPlan &Plan, VPValue &Idiom) {
+  auto *FoundWidenCanonicalIVUser =
----------------
nit: `replaceHeaderPredicateWithIdiom`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1027
+// replaces all uses of VPCanonicalIVPHIRecipe with a
+// VPExplicitVectorLengthPHIRecipe. The only user instructions are
+// CanonicalIVIncrement and branch-on-count. VPCanonicalIVPHIRecipe is used
----------------
nit: all uses except the canonical IV increment.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1028
+// VPExplicitVectorLengthPHIRecipe. The only user instructions are
+// CanonicalIVIncrement and branch-on-count. VPCanonicalIVPHIRecipe is used
+// only for loop iterations counting after this transformation.
----------------
nit: the only user is CanonicalIVIncrement I think, branch-on-count uses the increment.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1079
+  // VPExplicitVectorLengthPHIRecipe except for
+  // VPInstruction::CanonicalIVIncrement and VPCanonicalIVPHIRecipe itself.
+  for (VPUser *U : to_vector(CanonicalIVPHI->users())) {
----------------
The only users remaining should be CanonicalIVIncrement. Simpler to just do the below?


```lang=diff
   // Replace all uses of VPCanonicalIVPHIRecipe by
   // VPExplicitVectorLengthPHIRecipe except for
-  // VPInstruction::CanonicalIVIncrement and VPCanonicalIVPHIRecipe itself.
-  for (VPUser *U : to_vector(CanonicalIVPHI->users())) {
-    if (auto *I = dyn_cast<VPInstruction>(U);
-        I && I->getOpcode() == VPInstruction::CanonicalIVIncrement)
-      continue;
-    if (isa<VPCanonicalIVPHIRecipe>(U))
-      continue;
-    auto *UI = dyn_cast<VPRecipeBase>(U);
-    if (!UI)
-      continue;
-    for (unsigned Idx = 0, E = UI->getNumOperands(); Idx < E; ++Idx)
-      if (UI->getOperand(Idx) == CanonicalIVPHI)
-        UI->setOperand(Idx, EVLPhi);
-  }
-  // Cleanup dead recipes after the transformation.
-  removeDeadRecipes(Plan);
+  // VPInstruction::CanonicalIVIncrement.
+  CanonicalIVPHI->replaceAllUsesWith(EVLPhi);
+  CanonicalIVIncrement->setOperand(0, CanonicalIVPHI);
```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1093
   }
+  // Cleanup dead recipes after the transformation.
+  removeDeadRecipes(Plan);
----------------
This shouldn't introduce any new dead recipes, unless the canonical IV is only used to control the exit, so IIUC the only dead recipes introduced here should be due to `replacePredicateWithIdiom`. 

May be good to either clean up the recipes there or move `addVectorPredication` after `optimizeInductions`, but that would require passing in an extra flag to `optimize()`, so maybe better to clean up the dead recipes directly after removing their users.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:83
+  /// Add a VPExplicitVectorLengthPHIRecipe and related recipes to \p Plan and
+  /// replaces all uses of VPCanonicalIVPHIRecipe with a
+  /// VPExplicitVectorLengthPHIRecipe. The only user instructions are
----------------
nit: not all users, all users except the canonical IV increment, right?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:87
+  /// only for loop iterations counting after this transformation.
+  static void addVectorPredication(VPlan &Plan);
+
----------------
nit: `addExplicitVectorLength`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750