Page MenuHomePhabricator

[VPlan] Properly handle sinking of replicate regions.
ClosedPublic

Authored by fhahn on Apr 19 2021, 4:09 AM.

Details

Summary

This patch updates the code that sinks recipes required for first-order
recurrences to properly handle replicate-regions. At the moment, the
code would just move the replicate recipe out of its replicate-region,
producing an invalid VPlan.

When sinking a recipe in a replicate-region, we have to sink the whole
region. To do that, we first need to split the block at the target
recipe and move the region in between.

This patch also adds a splitAt helper to VPBasicBlock to split a
VPBasicBlock at a given iterator.

I think we also need to deal with the case when the target is a
replicate region also. I'll put up a patch once I constructed a test
for that scenario.

Fixes PR50009.

Diff Detail

Event Timeline

fhahn created this revision.Apr 19 2021, 4:09 AM
fhahn requested review of this revision.Apr 19 2021, 4:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2021, 4:09 AM
Herald added a subscriber: vkmr. · View Herald Transcript
fhahn updated this revision to Diff 338534.Apr 19 2021, 8:36 AM

Add missing tests.

Ayal accepted this revision.May 3 2021, 12:41 AM

This looks good, thanks for fixing, adding a couple of nits.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9103–9106

I think we also need to deal with the case when the target is a
replicate region also. I'll put up a patch once I constructed a test
for that scenario.

Agreed. Worth adding an assert in the meanwhile?

9145

better early-exit the simple Sink->moveAfter(Target) non-pred-rep case first?

llvm/lib/Transforms/Vectorize/VPlan.cpp
404

error message

llvm/lib/Transforms/Vectorize/VPlan.h
1535

mobing

This revision is now accepted and ready to land.May 3 2021, 12:41 AM
fhahn updated this revision to Diff 342859.May 4 2021, 2:13 PM

Address comments, thanks!

fhahn marked 3 inline comments as done.May 4 2021, 2:15 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9103–9106

That's a great idea, I added an assert.

This revision was landed with ongoing or failed builds.May 4 2021, 2:36 PM
This revision was automatically updated to reflect the committed changes.
fhahn added inline comments.May 4 2021, 2:37 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9103–9106

Actually this case is already handled, not sure if I was thinking about something else. I also updated the logic to check directly whether sink is in a replicate region, same as for target.

Ayal added inline comments.May 5 2021, 1:26 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9103–9106

Worth mentioning where the case having both sink and target predicated is handled(?)

Better rename Region >> TargetRegion.

fhahn added inline comments.May 5 2021, 1:48 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9103–9106

Ah right, I missed the *also* when re-reading. So the unhandled case is when both sink and target are in separate regions! Let me look into adding an assert for that and update the naming.

fhahn added inline comments.May 7 2021, 1:29 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9103–9106

I pushed 01c26d4e048c & 337d7652823f to rename the variable and add an extra assert.

ebrevnov added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9135

Hi Florian,

Just want to give you a heads up that we observe crash downstream at this line. In particular, std::next(Target->getIterator()) returns null. So far I have internal reproducer only. Maybe you can figure out the problem faster than I come up with upstreamable reproducer. Thanks in advance.

fhahn added inline comments.May 12 2021, 1:24 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9135

Thanks for the heads-up! I'll try to see if I can come up with a test case tomorrow.

fhahn added inline comments.May 13 2021, 5:39 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9135

I managed to construct a test case that requires splitting at the end of the block, which crashes at the moment, because the assert dereferences splitAt unconditionally. Should be fixed in bdada7546e6b

Please let me know if there's still an issue downstream.

ebrevnov added inline comments.May 13 2021, 5:52 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9135

Thanks for quick turn around. Will check and let you know.

ebrevnov added inline comments.Tue, May 25, 1:36 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9135

Thanks for quick turn around. Will check and let you know.

That does fix our issue. Thanks!