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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

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.



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


================
Comment at: lib/Transforms/Vectorize/VPlan.h:916
+  /// Return a reference to the list with recipes.
+  RecipeListTy &getRecipes() { return Recipes; }
+
----------------
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?


https://reviews.llvm.org/D46825