Page MenuHomePhabricator

[LV] Refactor ILV.vectorize[Loop]() by introducing LVP.executePlan()
ClosedPublic

Authored by Ayal on Apr 18 2017, 4:21 PM.

Details

Summary

Introduce LoopVectorizationPlanner.executePlan(), replacing ILV.vectorize() and refactoring ILV.vectorizeLoop(). collectDeadInstructions() is moved from ILV to LVP.
These will facilitate building VPlans and using them to generate code, following https://reviews.llvm.org/D28975 and its tentative breakdown.

Diff Detail

Repository
rL LLVM

Event Timeline

Ayal created this revision.Apr 18 2017, 4:21 PM
mkuper added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
395 ↗(On Diff #95643)

The public interface this ends up exposing is pretty weird, but I don't have any good ideas.
Matt, what do you think?

472 ↗(On Diff #95643)

Can you move this into the public part above, instead of adding one here?

mssimpso added inline comments.Apr 20 2017, 12:24 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
395 ↗(On Diff #95643)

I see what you mean, but I also don't have any real suggestions. Something like LV.vectorizeLoop(LVP.getBestVPlan()) might be more straightforward than LVP.executePlan(). But I'm not sure how such a change would affect the refactoring plans.

Ayal added inline comments.Apr 20 2017, 1:30 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
472 ↗(On Diff #95643)

Sure, will do.
This way hopefully conveyed the diff more easily.

Ayal added inline comments.Apr 21 2017, 10:08 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
395 ↗(On Diff #95643)

ILV.createEmptyLoop() should indeed better be renamed ILV.createVectorizedLoop().

The refactoring plans aim to employ ILV for code-generation services, such as vectorizeInstruction(), driven by VPlan.transform() via its Recipes. Similarly to later employing CM for cost-model services by VPlan.expectedCost(), thereby aligning the two. Invoking VPlan.transform() on the best VPlan should eventually take care of all code generation. Before having VPlan in place, covering all code generation, this patch proposes to have LVP.executePlan() take care of it.

collectTriviallyDeadInstructions() is moved to LVP, and will later serve it to construct VPlans; in this patch it serves LVP to generate the code.

Ayal updated this revision to Diff 96370.Apr 24 2017, 3:32 AM

Patch updated following review comments. Better/ok now?

Hi Ayal,

I like where this is going, particularly the executePlan idiom. I don't think it's a really big deal now how we organise the interface, given that it's going to change (probably radically) once VPlans start to trickle in.

A few silly comments on comments, but otherwise, it's looking good. Thanks!

I'll let @mssimpso and @mkuper finish their reviews. :)

cheers,
--renato

lib/Transforms/Vectorize/LoopVectorize.cpp
394 ↗(On Diff #96370)

This also vectorises, no? This comment implies it just replaces with an empty loop.

7531 ↗(On Diff #96370)

This is not strictly true, as it could be unrolling, right?

Maybe you should say "loop transformation"?

Ayal added inline comments.May 3 2017, 12:59 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
394 ↗(On Diff #96370)

Ahh, no, it creates an empty loop into which vector code will subsequently be inserted (by calls to vectorizeInstruction()), and introduces its scalar induction variable of step VF*UF. It also creates all the surrounding blocks, branches, etc.. This was the exact original behavior of the method, and its original comment. This patch only exposes it publicly, to be (re)used for generating code by vplan.

Perhaps a better yet name for it may be createVectorizedLoopSkeleton(). Can we agree on this, or other welcomed suggestions?

7531 ↗(On Diff #96370)

Right, executing a Plan follows the selected VF and UF, which may end up producing vector and/or unrolled code.

This is the original comment taken from ILV::vectorize(), which also serves the Unroller.

Agreed, we should say "loop transformation" instead.

rengolin added inline comments.May 4 2017, 2:39 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
394 ↗(On Diff #96370)

Right, I see. I'm ok with whatever name that reflects that this is just the new empty loop. Not all TAB-completion bring the comments above the function. :)

Ayal added a comment.May 10 2017, 8:38 AM

Ping.

https://reviews.llvm.org/D32871 includes an updated version of this patch. It will be updated once this patch is approved and committed.

mssimpso accepted this revision.May 10 2017, 9:07 AM

Hey Ayal,

Sorry to lose track of this patch. It looks good to me.

lib/Transforms/Vectorize/LoopVectorize.cpp
7880 ↗(On Diff #96370)

Do we know how this ended up begin "LB"? We might should change it to something more meaningful at some point (not in this patch, though).

This revision is now accepted and ready to land.May 10 2017, 9:07 AM
This revision was automatically updated to reflect the committed changes.