Page MenuHomePhabricator

[LV] Introduce VPBlendRecipe, VPWidenMemoryInstructionRecipe
ClosedPublic

Authored by gilr on Oct 18 2017, 1:00 PM.

Details

Summary

This patch is part of D38676.

The patch introduces two new Recipes to handle instructions whose vectorization involves masking. These Recipes take VPlan-level masks in D38676, but still rely on ILV's existing createEdgeMask(), createBlockInMask() in this patch.

  • VPBlendRecipe handles intra-loop phi nodes, which are vectorized as a sequence of SELECTs. Its execute() code is refactored out of ILV::widenPHIInstruction(), which now handles only loop-header phi nodes.
  • VPWidenMemoryInstructionRecipe handles load/store which are to be widened (but are not part of an Interleave Group). In this patch it simply calls ILV::vectorizeMemoryInstruction on execute().

Note that ILV methods called by these Recipes were made public.

Diff Detail

Repository
rL LLVM

Event Timeline

gilr created this revision.Oct 18 2017, 1:00 PM
rengolin edited edge metadata.Oct 28 2017, 9:06 AM

The patch is much easier to review now, mostly code movement. Thanks!

Overall, it looks good and is on par with what I was expecting to see when moving internal recipes to VPlans.

I strongly believe we shouldn't be making methods public, even if just while moving to VPlans.

Given that we are now asserting when seeing memory instructions in the generic widener (a thing that we may drop as soon as we move all recipes out, and just make it a generic assert for all non handled instruction types), I believe it should be fine to move the code inside the plans themselves, instead of creating the call backs.

Who else calls vectorizeMemoryInstruction or the others? It would be good to know and make sure they don't (or why they do and create tasks to clear that up).

cheers,
--renato

lib/Transforms/Vectorize/LoopVectorize.cpp
433 ↗(On Diff #119472)

IMHO, we shouldn't make methods public just because we need them from outside. Protection levels should be selected based on what they do, not who may use them.

To me, the need to move them public is indication that the design needs more thought. I'm ok with the thought and implementation being in another patch, but making them public here may encourage bad practices elsewhere.

If anything else fails, we probably should make the recipes friend instead, with a very big warning comment that we need to think more about the overall design.

4738 ↗(On Diff #119472)

nit: I'd mention "elsewhere" is its own recipe.

7966 ↗(On Diff #119472)

Maybe not for this patch, but I think the logic should be moved inside this class, and not be a call back to the ILV.

gilr added inline comments.Oct 30 2017, 8:33 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
433 ↗(On Diff #119472)

Under VPlan, ILV is transitioning into a provider of code-gen services to the Planner and Recipes. It seems reasonable to separate the services it provides, which is what it does, from its internal utilities.

While the public/friend design discussion is valid, ILV currently has a public section and OTOH no friends so befriending these two Recipes with it would make an exception in the current design.

4738 ↗(On Diff #119472)

Ok, will merge this with the default case.

7966 ↗(On Diff #119472)

Right. In general, ILV's code-gen methods should be refactored such that the decision-taking logic is moved to the Planner and the IR generation is moved to the relevant Recipe's execute() method (as applied to createBlockInMask() & createEdgeMask() in D38676).
Agreed, not for this patch.

rengolin added inline comments.Oct 30 2017, 11:09 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
433 ↗(On Diff #119472)

I agree on two points:

  1. The discussion between public/friend (something else) doesn't need to be done in this patch, but it does need to happen.
  2. It's good to separate provider/user, but the way we're going, the public methods in ILV could unintentionally create coupling between recipes. (true, friend/public won't change solve that problem).

I worry more about (2) than (1), and that's why I think this discussion can happen outside of this patch.

Once ILV becomes an empty shell of utility methods, we should consider moving all services to LoopUtils.

fhahn added a subscriber: fhahn.Oct 31 2017, 2:21 AM
gilr updated this revision to Diff 121497.Nov 3 2017, 10:03 AM

Addressed review comment - remove the load/store specialized llvm_unreachable from ILV::widenInstruction, load/store now also handled by the switch's default case.

Excellent, ok, this patch is looking good to me now. How does this fit into the whole story?

The other review (D38676) still has this patch inside, plus tests. Are you going to clean that up and move the tests here?

It'd be good to have only the refactoring part there, and only the new plans here, with tests.

gilr added a comment.Nov 7 2017, 6:55 AM

Excellent, ok, this patch is looking good to me now. How does this fit into the whole story?

The other review (D38676) still has this patch inside, plus tests. Are you going to clean that up and move the tests here?

It'd be good to have only the refactoring part there, and only the new plans here, with tests.

Ah, good catch - the LIT test case added in D38676 actually belongs here too, as it makes sure the new tryToWidenMemory() takes sinkScalarOperands into account. Will transfer it to this patch. The slight modification to the other LIT in D38676 is related to the swtich to VPInstructions for masks which slightly change order in which the IR is generated.

An important part of D38676 is the refactoring of createBlockInMask() and createEdgeMask() to use VPValues instead of Values as the move from ILV to Planner. Relocating those methods in this patch to their would-be location in D38676 in an NFC for this patch and should greatly facilitate reviewing the changes made to those two methods. This will also be in the coming revision of this patch.
I'll rebase D38676 on top of this patch - the diff in LoopVectorize.cpp will be smaller and hopefully more tightly related to the new VPInstruction infrastructure.

gilr updated this revision to Diff 121895.Nov 7 2017, 7:26 AM
  • Added LIT test case to make sure the new tryToWidenMemory() method takes scalarization decisions into account.
  • NFC: move createBlockInMask(), createEdgeMask() to their would-be location in D38676 as part of the Planner's methods to facilitate their review.
rengolin accepted this revision.Nov 10 2017, 9:25 PM

LGTM, thanks!

This revision is now accepted and ready to land.Nov 10 2017, 9:25 PM
This revision was automatically updated to reflect the committed changes.