This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Move recipe construction to VPRecipeBuilder.
ClosedPublic

Authored by fhahn on May 31 2018, 10:03 AM.

Details

Summary

This should allow us to re-use the functionality in a
VPInstructionToVPRecipe transformation. I think it also makes sense to
move those functions out of LoopVectorizationPlanner, which should do
the high-level orchestration of the transformations.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.May 31 2018, 10:03 AM

Thanks for doing this refactoring! In general, moving all this code outside of the planner makes sense to me.
create*Mask and getDecisionAndClampRange are not strictly creating recipes but I understand the dependencies.
Minor comments inline.

Thanks,
Diego

lib/Transforms/Vectorize/LoopVectorize.cpp
6481 ↗(On Diff #149311)

Not a big deal but I wonder if it would be better to move this outside of VPRecipeBuilder since it's not strictly creating or manipulating any recipe and it will be needed eventually in the VPlan native path.

lib/Transforms/Vectorize/VPRecipeBuilder.h
25 ↗(On Diff #149311)

Add brief description of this class?

79 ↗(On Diff #149311)
// -> ///?
lib/Transforms/Vectorize/VPlan.h
72 ↗(On Diff #149311)

OK, this makes sense to me.

fhahn updated this revision to Diff 149490.Jun 1 2018, 8:56 AM
fhahn marked 2 inline comments as done.

Thanks for having a look Diego!

lib/Transforms/Vectorize/LoopVectorize.cpp
6481 ↗(On Diff #149311)

Yep I think it would be better to move it somewhere else. I've moved it back to a static method in LoopVectorizationPlanner. Do you think there's a better place?

dcaballe accepted this revision.Jun 1 2018, 9:14 AM

Thanks, Florian! LGTM!
Please wait until we finish the other two related reviews to commit it, in case we have to change something else.

Thanks,
Diego

lib/Transforms/Vectorize/LoopVectorize.cpp
6481 ↗(On Diff #149311)

I think it's OK for now. We already have a VPBlockUtils and I envision a VPInstructionUtils. We should also have something similar for more generic stuff but we can introduce it when we have a clearer picture of it.

This revision is now accepted and ready to land.Jun 1 2018, 9:14 AM
fhahn added a comment.Jun 4 2018, 4:42 AM

Will do, thanks for having a look Diego!

lib/Transforms/Vectorize/LoopVectorize.cpp
6481 ↗(On Diff #149311)

Yep I think that will be helpful in the future.

fhahn added a comment.Jun 6 2018, 10:26 AM

Thanks, Florian! LGTM!
Please wait until we finish the other two related reviews to commit it, in case we have to change something else.

As I updated D46827 to not rely on VPRecipeBuilder, but instead create a limited subset of recipes directly, do you think this is good to go?

Thanks, Florian! LGTM!
Please wait until we finish the other two related reviews to commit it, in case we have to change something else.

As I updated D46827 to not rely on VPRecipeBuilder, but instead create a limited subset of recipes directly, do you think this is good to go?

Yes, this refactoring still makes sense. OK, go ahead and commit it!

This revision was automatically updated to reflect the committed changes.