[VPlan] Add moveAfter to VPRecipeBase.

[fhahn]

Summary

This patch adds a moveAfter method to VPRecipeBase, which can be used to
move elements after other elements, across VPBasicBlocks, if necessary.

It also adds an initial unittest for VPlan.

[dcaballe]

Thanks for the unittest, Florian! I will be very useful!

lib/Transforms/Vectorize/VPlan.h

543 ↗

(On Diff #146572)

instruction -> recipe
VPBasicBloack -> VPBasicBlock

I wonder if it would be better to start a VPInstructionUtils and move this utility there, similar to what I did with VPBlockUtils in D44338.

916 ↗

(On Diff #146572)

Just for my understanding, wouldn't it be possible to use getSublistAccess?

[fhahn]

Thanks Diego. I think we should be able to use unit tests extensively for VPlan, as it should be quite lightweight to construct a VPlan, run a transformation and check the resulting VPlan.

lib/Transforms/Vectorize/VPlan.h

543 ↗

(On Diff #146572)

I added it as member function here because there is a similar Instruction::moveAfter function. I think having a similar member function here makes it easier for people being used to the existing APIs.

916 ↗

(On Diff #146572)

Yes I guess we could use getSublistAccess, but I am not entirely sure what the use case of this function is? Its comment says it returns a pointer to a member of the recipe list, but then it ignores the parameter.

IMO it would be better, safer to return a reference (BasicBlock has a similar getInstList function). Maybe getRecipes should be renamed to getRecipeList and we could remove getSublistAccess, as it is not used anywhere?

[fhahn]

Thank

[dcaballe]

Only minor comments! LGTM.
Thanks!

lib/Transforms/Vectorize/VPlan.h

543 ↗

(On Diff #146572)

I think having a similar member function here makes it easier for people being used to the existing APIs.

Ok, if that's the case for Instruction let's go with this.

916 ↗

(On Diff #146572)

Agreed. Yes, please, remove getSublistAccess and let's use getRecipeList.
Thanks!

unittests/Transforms/Vectorize/VPlanTest.cpp

26 ↗

(On Diff #146826)

delete all the new Instructions and VPInstruction will also be necessary here?

[dcaballe]

Just make sure that in the final version of D46827 this code is still necessary.
Thanks!

[fhahn]

After the recent changes to D46827, this patch is not really necessary for now, but it may be useful in the future.

llvm/lib/Transforms/Vectorize/VPlan.h

public:
 
   /// Each recipe prints itself.
   virtual void print(raw_ostream &O, const Twine &Indent) const = 0;
 
   /// Insert an unlinked recipe into a basic block immediately before
   /// the specified recipe.
   void insertBefore(VPRecipeBase *InsertPos);
 
+  /// Unlink this recipe from its current VPBasicBlock and insert it into
+  /// the VPBasicBlock that MovePos lives in, right after MovePos.
+  void moveAfter(VPRecipeBase *MovePos);
+
   /// This method unlinks 'this' from the containing basic block and deletes it.
   ///
   /// \returns an iterator pointing to the element after the erased one
   iplist<VPRecipeBase>::iterator eraseFromParent();
 };
 
 /// This is a concrete Recipe that models a single VPlan-level instruction.
 /// While as any Recipe it may generate a sequence of IR instructions when

llvm/lib/Transforms/Vectorize/VPlan.cpp

void VPRecipeBase::insertBefore(VPRecipeBase *InsertPos) {
   Parent = InsertPos->getParent();
   Parent->getRecipeList().insert(InsertPos->getIterator(), this);
 }
 
 iplist<VPRecipeBase>::iterator VPRecipeBase::eraseFromParent() {
   return getParent()->getRecipeList().erase(getIterator());
 }
 
+void VPRecipeBase::moveAfter(VPRecipeBase *InsertPos) {
+  InsertPos->getParent()->getRecipeList().splice(
+      std::next(InsertPos->getIterator()), getParent()->getRecipeList(),
+      getIterator());
+}
+
 void VPInstruction::generateInstruction(VPTransformState &State,
                                         unsigned Part) {
   IRBuilder<> &Builder = State.Builder;
 
   if (Instruction::isBinaryOp(getOpcode())) {
     Value *A = State.get(getOperand(0), Part);
     Value *B = State.get(getOperand(1), Part);
     Value *V = Builder.CreateBinOp((Instruction::BinaryOps)getOpcode(), A, B);

llvm/unittests/Transforms/Vectorize/VPlanTest.cpp

TEST(VPInstructionTest, eraseFromParent) {
 
   I1->eraseFromParent();
   CHECK_ITERATOR(VPBB1, I3);
 
   I3->eraseFromParent();
   EXPECT_TRUE(VPBB1.empty());
 }
 
+TEST(VPInstructionTest, moveAfter) {
+  VPInstruction *I1 = new VPInstruction(0, {});
+  VPInstruction *I2 = new VPInstruction(1, {});
+  VPInstruction *I3 = new VPInstruction(2, {});
+
+  VPBasicBlock VPBB1;
+  VPBB1.appendRecipe(I1);
+  VPBB1.appendRecipe(I2);
+  VPBB1.appendRecipe(I3);
+
+  I1->moveAfter(I2);
+
+  CHECK_ITERATOR(VPBB1, I2, I1, I3);
+
+  VPInstruction *I4 = new VPInstruction(4, {});
+  VPInstruction *I5 = new VPInstruction(5, {});
+  VPBasicBlock VPBB2;
+  VPBB2.appendRecipe(I4);
+  VPBB2.appendRecipe(I5);
+
+  I3->moveAfter(I4);
+
+  CHECK_ITERATOR(VPBB1, I2, I1);
+  CHECK_ITERATOR(VPBB2, I4, I3, I5);
+}
+
 } // namespace
 } // namespace llvm