Page MenuHomePhabricator

[VPlan] Add moveAfter to VPRecipeBase.
AcceptedPublic

Authored by fhahn on May 14 2018, 3:54 AM.

Details

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.

Diff Detail

Event Timeline

fhahn created this revision.May 14 2018, 3:54 AM

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

lib/Transforms/Vectorize/VPlan.h
543

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

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

fhahn added a comment.May 15 2018, 7:27 AM

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

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

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?

dcaballe accepted this revision.May 15 2018, 7:54 PM

Only minor comments! LGTM.
Thanks!

lib/Transforms/Vectorize/VPlan.h
543

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

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

unittests/Transforms/Vectorize/VPlanTest.cpp
26

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

This revision is now accepted and ready to land.May 15 2018, 7:54 PM

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

fhahn added a comment.Jun 14 2018, 5:22 AM

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