Page MenuHomePhabricator

[CodeExtractor] Restore outputs after creating exit stubs

Authored by kachkov98 on Feb 7 2019, 11:45 AM.



When CodeExtractor saves the result of InvokeInst at the first insertion point of the 'normal destination' basic block, this block can be omitted in the outlined region, so store is placed outside of the function. The suggested solution is to process saving outputs after creating exit stubs for new function, and stores will be placed in that blocks before return in this case.

Diff Detail


Event Timeline

kachkov98 created this revision.Feb 7 2019, 11:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 11:45 AM
vsk added a comment.Feb 7 2019, 12:04 PM

Thanks! This mostly looks good, and I believe it resolves

1141 ↗(On Diff #185834)

Could you assert that the insertion point is in 'newFunction'?

203 ↗(On Diff #185834)

Are the 'entry' and 'cond.true' blocks needed to reproduce the issue?

231 ↗(On Diff #185834)

Is DT needed?

kachkov98 marked 3 inline comments as done.Feb 8 2019, 12:38 AM
kachkov98 added inline comments.
1141 ↗(On Diff #185834)

At this point the extracted blocks are still in the old function, and exit stubs are created immediately in new function; so insertion point should be in newFunction or in one of extracted blocks.

203 ↗(On Diff #185834)

Agree that test can be simplified.

231 ↗(On Diff #185834)

It seems that dominator tree is only used to verify that all blocks are reachable from entry, so I think it can be safely removed from all CodeExtractor tests.

kachkov98 updated this revision to Diff 185921.Feb 8 2019, 12:40 AM

Add assertion for the InsertPt
Remove DT construction from CodeExtractor tests
Simplify test

vsk accepted this revision.Feb 8 2019, 10:54 AM

Thanks for doing this! LGTM with a simple change.

1142 ↗(On Diff #185921)

It looks like we always deref InsertPt below. Can we just define 'InsertBefore = &*InsertPt' once, and then use it in the assert? Also, it might be simpler to write InsertBefore->getFunction().

This revision is now accepted and ready to land.Feb 8 2019, 10:54 AM
kachkov98 updated this revision to Diff 186022.Feb 8 2019, 11:55 AM

Thank you! Could you please commit it? (I don't have access to do it)

This revision was automatically updated to reflect the committed changes.