[PATCH] D46825: [VPlan] Add moveAfter to VPRecipeBase.

[Diego Caballero via Phabricator via llvm-commits]

dcaballe accepted this revision.
dcaballe added a comment.
This revision is now accepted and ready to land.

Only minor comments! LGTM.
Thanks!



================
Comment at: lib/Transforms/Vectorize/VPlan.h:543
+  /// the VPBasicBloack that MovePos lives in, right after MovePos.
+  void moveAfter(VPRecipeBase *MovePos);
 };
----------------
fhahn wrote:
> dcaballe wrote:
> > 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`.
> 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. 
> 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.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:916
+  /// Return a reference to the list with recipes.
+  RecipeListTy &getRecipes() { return Recipes; }
+
----------------
fhahn wrote:
> dcaballe wrote:
> > Just for my understanding, wouldn't it be possible to use `getSublistAccess`?
> 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?
Agreed. Yes, please, remove `getSublistAccess` and let's use `getRecipeList`.
Thanks!


================
Comment at: unittests/Transforms/Vectorize/VPlanTest.cpp:26
+TEST(VPInstructionTest, moveAfter) {
+  VPInstruction *I1 = new VPInstruction(0, {});
+  VPInstruction *I2 = new VPInstruction(1, {});
----------------
delete all the new Instructions and VPInstruction will also be necessary here?


https://reviews.llvm.org/D46825