This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Make sure recurrence splice is not inserted between phis.
ClosedPublic

Authored by fhahn on Oct 7 2021, 4:25 AM.

Details

Summary

All phi-like recipes should be at the beginning of a VPBasicBlock with
no other recipes in between. Ensure that the recurrence-splicing recipe
is not added between phi-like recipes, but after them.

Diff Detail

Event Timeline

fhahn created this revision.Oct 7 2021, 4:25 AM
fhahn requested review of this revision.Oct 7 2021, 4:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2021, 4:25 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added inline comments.Oct 25 2021, 2:18 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9507

Hmm, PrevRecipe cannot be a header phi because that would result in 2nd order recurrence (or greater); but it may be a phi of a replicated region? Should blends be excluded here? Curious to see a test...

9511

How about setting the insertion point of Builder before creating the Op, instead of finding where to move it afterwards?

fhahn updated this revision to Diff 382377.Oct 26 2021, 9:51 AM
fhahn marked an inline comment as done.

Updated to set the insert point of the builder up-front, rather than moving the created recipies later. Also removed unnneded isPhi check.

fhahn marked an inline comment as done.Oct 26 2021, 9:52 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9507

I thought there was a test case, but it looks like I remembered incorrectly. Removed the check.

9511

How about setting the insertion point of Builder before creating the Op, instead of finding where to move it afterwards?

Sounds good, updated!

Ayal added inline comments.Oct 26 2021, 12:05 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9506

This became an NFC patch, update its title and summary?
Assert that PrevRecipe is not a Phi, or have the verifier catch if it is (a Phi but not the last Phi)?

fhahn retitled this revision from [VPlan] Make sure recurrence splice is not inserted between phis. to [VPlan] Set builder insert point instead of moving recipes. (NFC).Oct 27 2021, 3:07 AM
fhahn edited the summary of this revision. (Show Details)
fhahn marked an inline comment as done.Oct 27 2021, 3:12 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9507

I thought there was a test case, but it looks like I remembered incorrectly. Removed the check.

Ok I now what phi-like case I originally hit: if the previous value is an induction value! So I think we still need the special handling?

fhahn updated this revision to Diff 382602.Oct 27 2021, 4:24 AM

Restore code to set insert point for phi-like recipes.

Relevant test is something like trunc_with_first_order_recurrence in
test/Transforms/LoopVectorize/induction.ll

fhahn retitled this revision from [VPlan] Set builder insert point instead of moving recipes. (NFC) to [VPlan] Make sure recurrence splice is not inserted between phis..Oct 27 2021, 4:24 AM
fhahn edited the summary of this revision. (Show Details)
fhahn added a comment.Nov 4 2021, 5:20 AM

ping :)

@Ayal does the clarification why this is needed make sense?

(from inline comment)
Ok I now what phi-like case I originally hit: if the previous value is an induction value! So I think we still need the special handling?

Ayal added a comment.Nov 4 2021, 9:16 AM

ping :)

@Ayal does the clarification why this is needed make sense?

(from inline comment)
Ok I now what phi-like case I originally hit: if the previous value is an induction value! So I think we still need the special handling?

Clarification indeed makes sense, induction feeding a FOR should presumably be handled, not being a 2nd-order recurrence (SOR).

So we may indeed still need the special handling - along with special test(s); perhaps using i-1 inside a "for i" loop? Would also be good to address/exclude/cover non-header phis - of replicated regions and blends.

fhahn added a comment.Nov 5 2021, 1:30 PM

ping :)

@Ayal does the clarification why this is needed make sense?

(from inline comment)
Ok I now what phi-like case I originally hit: if the previous value is an induction value! So I think we still need the special handling?

Clarification indeed makes sense, induction feeding a FOR should presumably be handled, not being a 2nd-order recurrence (SOR).

So we may indeed still need the special handling - along with special test(s); perhaps using i-1 inside a "for i" loop?

There should be test coverage already for those cases, in Transforms/LoopVectorize/induction.ll, which fails verification from D111302 without the current patch.

Would also be good to address/exclude/cover non-header phis - of replicated regions and blends.

Should those be included in this patch here?

Ayal accepted this revision.Nov 8 2021, 3:05 AM

ping :)

@Ayal does the clarification why this is needed make sense?

(from inline comment)
Ok I now what phi-like case I originally hit: if the previous value is an induction value! So I think we still need the special handling?

Clarification indeed makes sense, induction feeding a FOR should presumably be handled, not being a 2nd-order recurrence (SOR).

So we may indeed still need the special handling - along with special test(s); perhaps using i-1 inside a "for i" loop?

There should be test coverage already for those cases, in Transforms/LoopVectorize/induction.ll, which fails verification from D111302 without the current patch.

ok; can a comment be added to these tests, mentioning that they exercise induction-feeding-a-FOR, and careful placement of the FOR splice?
Wonder how VPlan generated correct code, with splice placed in the midst of phi's(?)

Would also be good to address/exclude/cover non-header phis - of replicated regions and blends.

Should those be included in this patch here?

or left as a TODO?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9491–9501

nit: can tentatively set

VPBasicBlock *InsertBlock = PrevRecipe->getParent();

initially, and later override it with

InsertBlock = cast<VPBasicBlock>(Region->getSingleSuccessor());
This revision is now accepted and ready to land.Nov 8 2021, 3:05 AM
fhahn marked an inline comment as done.Nov 8 2021, 9:48 AM

ping :)

@Ayal does the clarification why this is needed make sense?

(from inline comment)
Ok I now what phi-like case I originally hit: if the previous value is an induction value! So I think we still need the special handling?

Clarification indeed makes sense, induction feeding a FOR should presumably be handled, not being a 2nd-order recurrence (SOR).

So we may indeed still need the special handling - along with special test(s); perhaps using i-1 inside a "for i" loop?

There should be test coverage already for those cases, in Transforms/LoopVectorize/induction.ll, which fails verification from D111302 without the current patch.

ok; can a comment be added to these tests, mentioning that they exercise induction-feeding-a-FOR, and careful placement of the FOR splice?
Wonder how VPlan generated correct code, with splice placed in the midst of phi's(?)

Done in the committed version. The issue in the test case were other induction recipes after the splice. The existing widenIntOrFpInduction inserts new phis at the beginning of the BB, so it did not cause any issues during actual codegen.

Would also be good to address/exclude/cover non-header phis - of replicated regions and blends.

Should those be included in this patch here?

or left as a TODO?

I did not add a TODO in the committed version, because unfortunately I am not sure where to best add one :(

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9491–9501

Done in the committed version, thanks!