Page MenuHomePhabricator

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

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



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 and its tentative breakdown.

Diff Detail


Event Timeline

Ayal created this revision.Apr 18 2017, 4:21 PM
mkuper added inline comments.
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
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
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
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. :)


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
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
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. 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.

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.