[PATCH] D68577: [LV] Apply sink-after & interleave-groups as VPlan transformations (NFC)

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6903
+
+  if (!Recipe)
+    Recipe = tryToBlend(Instr, Plan);
----------------
This if (!Recipe) case and the next should be nested?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6917
   // having first checked for specific widening recipes that deal with
   // Interleave Groups, Inductions and Phi nodes.
   if (tryToWiden(Instr, VPBB, Range))
----------------
Update above comment: we no longer check for Interleave Groups widening recipe here, only Inductions and Phi nodes.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7054
 
-      // I is a member of an InterleaveGroup for Range.Start. If it's an adjunct
-      // member of the IG, do not construct any Recipe for it.
-      const InterleaveGroup<Instruction> *IG =
-          CM.getInterleavedAccessGroup(Instr);
-      if (IG && Instr != IG->getInsertPos() &&
-          Range.Start >= 2 && // Query is illegal for VF == 1
-          CM.getWideningDecision(Instr, Range.Start) ==
-              LoopVectorizationCostModel::CM_Interleave) {
-        auto SinkCandidate = SinkAfterInverse.find(Instr);
-        if (SinkCandidate != SinkAfterInverse.end())
-          Ingredients.push_back(SinkCandidate->second);
-        continue;
-      }
-
-      // Move instructions to handle first-order recurrences, step 1: avoid
-      // handling this instruction until after we've handled the instruction it
-      // should follow.
-      auto SAIt = SinkAfter.find(Instr);
-      if (SAIt != SinkAfter.end()) {
-        LLVM_DEBUG(dbgs() << "Sinking" << *SAIt->first << " after"
-                          << *SAIt->second
-                          << " to vectorize a 1st order recurrence.\n");
-        SinkAfterInverse[SAIt->second] = Instr;
-        continue;
-      }
-
-      Ingredients.push_back(Instr);
-
-      // Move instructions to handle first-order recurrences, step 2: push the
-      // instruction to be sunk at its insertion point.
-      auto SAInvIt = SinkAfterInverse.find(Instr);
-      if (SAInvIt != SinkAfterInverse.end())
-        Ingredients.push_back(SAInvIt->second);
-    }
-
-    // Introduce each ingredient into VPlan.
-    for (Instruction *Instr : Ingredients) {
-      if (RecipeBuilder.tryToCreateRecipe(Instr, Range, Plan, VPBB))
-        continue;
+      bool Widened = RecipeBuilder.tryToCreateRecipe(Instr, Range, Plan, VPBB);
 
----------------
Can retain the early-exiting "if (tryToCreateRecipe()) continue"?


================
Comment at: llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h:53
+  // those ingredients get a VPWidenRecipe, also avoid compressing other
+  // ingredients into it to avoid having to split such receipes later.
+  DenseMap<Instruction *, VPRecipeBase *> Ingredient2Recipe;
----------------
receipes >> recipes 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:117
+}
+
 BasicBlock *
----------------
Better place the implementation of removeFromParent() next to that of eraseFromParent() below.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:288
+/// Insert an unlinked instruction into a basic block immediately after the
+/// specified instruction.
+void VPRecipeBase::insertAfter(VPRecipeBase *InsertPos) {
----------------
Above comment suffices at header file only (and strictly speaking it's about Recipes rather than instructions).

Update Parent as done in insertBefore() above.

Should (existing) insertBefore() also assert !Parent before setting it?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:611
+  void removeFromParent();
+
   /// The method which generates the output IR instructions that correspond to
----------------
Better place removeFromParent() right before eraseFromParent() below, to emphasize that the latter (only) also deletes it.


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

https://reviews.llvm.org/D68577