Page MenuHomePhabricator

[VPlan] Properly handle sinking of replicate regions.

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



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.Mon, Apr 19, 4:09 AM
fhahn requested review of this revision.Mon, Apr 19, 4:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Apr 19, 4:09 AM
Herald added a subscriber: vkmr. · View Herald Transcript
fhahn updated this revision to Diff 338534.Mon, Apr 19, 8:36 AM

Add missing tests.

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

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


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?


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


error message



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

Address comments, thanks!

fhahn marked 3 inline comments as done.Tue, May 4, 2:15 PM
fhahn added inline comments.

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

This revision was landed with ongoing or failed builds.Tue, May 4, 2:36 PM
This revision was automatically updated to reflect the committed changes.
fhahn added inline comments.Tue, May 4, 2:37 PM

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.Wed, May 5, 1:26 PM

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

Better rename Region >> TargetRegion.

fhahn added inline comments.Wed, May 5, 1:48 PM

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.Fri, May 7, 1:29 PM

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

ebrevnov added inline comments.

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.Wed, May 12, 1:24 PM

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

fhahn added inline comments.Thu, May 13, 5:39 AM

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.Thu, May 13, 5:52 AM

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