This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Part 7: Split coroutine into subfunctions
ClosedPublic

Authored by GorNishanov on Aug 12 2016, 11:48 AM.
Tokens
"Yellow Medal" token, awarded by GorNishanov.

Details

Summary

This patch adds simple coroutine splitting logic to CoroSplit pass.

Documentation and overview is here: http://llvm.org/docs/Coroutines.html.

Upstreaming sequence (rough plan)
1.Add documentation. (https://reviews.llvm.org/D22603)
2.Add coroutine intrinsics. (https://reviews.llvm.org/D22659)
...

  1. Split coroutine into subfunctions <= we are here
  2. Coroutine Frame Building algorithm
  3. Handle coroutine with unwinds

10+. The rest of the logic

Diff Detail

Repository
rL LLVM

Event Timeline

GorNishanov retitled this revision from to [Coroutines] Part 7: Split coroutine into subfunctions.
GorNishanov updated this object.
GorNishanov added a reviewer: majnemer.
GorNishanov added a subscriber: llvm-commits.

Remove PermissiveArrayRef and inline replaceAndRemove into its callers. Saves us about 15 lines.

majnemer added inline comments.Aug 12 2016, 3:10 PM
lib/Transforms/Coroutines/CoroCleanup.cpp
30–37 ↗(On Diff #67875)

I'd just make simplifyFunctionCFG a non-internal function and call it instead of making a new FPM.

44 ↗(On Diff #67875)

auto *

44 ↗(On Diff #67875)

Should this be a CallSite instead?

lib/Transforms/Coroutines/CoroEarly.cpp
52 ↗(On Diff #67875)

Pointers lean right.

lib/Transforms/Coroutines/CoroFrame.cpp
29 ↗(On Diff #67875)

auto *

30 ↗(On Diff #67875)

You could use &BB->front().

46 ↗(On Diff #67875)

Please have a period at the end of this comment.

63–66 ↗(On Diff #67875)

auto *

68 ↗(On Diff #67875)

We aren't huge users of std::numeric_limits, UINT32_MAX is a little more terse.

101 ↗(On Diff #67875)

auto *

109 ↗(On Diff #67875)

auto *

lib/Transforms/Coroutines/Coroutines.cpp
196 ↗(On Diff #67875)

auto * although maybe we should use a CallSite?

Feedback implemented.

GorNishanov marked 9 inline comments as done.Aug 12 2016, 4:51 PM
GorNishanov added inline comments.
lib/Transforms/Coroutines/CoroCleanup.cpp
30–37 ↗(On Diff #67875)

We may end up needed more cleanup passes there. I'll keep at is as is for now.

44 ↗(On Diff #67875)

The only throwing coroutine intrinsics are coro.resume and coro.destroy are lowered in coro.early.

lib/Transforms/Coroutines/CoroFrame.cpp
63–66 ↗(On Diff #67875)

Missed those. Will upload in a sec.

lib/Transforms/Coroutines/Coroutines.cpp
196 ↗(On Diff #67875)

Only coro.resume and coro.destroy are throwing and they are replaced by CoroEarly Pass.

GorNishanov marked 3 inline comments as done.
GorNishanov marked 5 inline comments as done.

All feedback implemented or rejected :)

majnemer added inline comments.Aug 15 2016, 7:51 PM
lib/Transforms/Coroutines/CoroFrame.cpp
22 ↗(On Diff #67940)

Do we still need this include?

104 ↗(On Diff #67940)

auto *

lib/Transforms/Coroutines/CoroSplit.cpp
156 ↗(On Diff #67940)

A comment should be a sentence: start with a cap letter and end with a period.

lib/Transforms/Coroutines/Coroutines.cpp
204–206 ↗(On Diff #67940)

This is not true, the basic blocks can be in a jumbled order. If you want to do this sort of check, you need to use a depth first iterator like depth_first and turn this assert into a report_fatal_error.

Alternatively, you can save the coro.frame you come across for later processing instead of a dfs walk.

209 ↗(On Diff #67940)

Ditto.

226–227 ↗(On Diff #67940)

I think this should be a report_fatal_error because nothing prohibits it upfront.

reworked coro::Shape::buildFrom(Function& F) to address the feedback
+ little nits fixed

GorNishanov marked 6 inline comments as done.Aug 15 2016, 9:22 PM
majnemer added inline comments.Aug 15 2016, 10:30 PM
lib/Transforms/Coroutines/CoroFrame.cpp
26–44 ↗(On Diff #67940)

These functions seem dead.

lib/Transforms/Coroutines/CoroSplit.cpp
132 ↗(On Diff #67940)

Caps + period.

156 ↗(On Diff #68130)

Hmm, do you have a comment somewhere explaining why? Is it expected that there are no meaningful users of the args?

172 ↗(On Diff #68130)

Period.

174–175 ↗(On Diff #68130)

You could use changeToUnreachable.

185 ↗(On Diff #68130)

Ditto.

271 ↗(On Diff #68130)

Args.empty().

281 ↗(On Diff #68130)

Period.

313 ↗(On Diff #68130)

Caps + period.

lib/Transforms/Coroutines/Coroutines.cpp
215–217 ↗(On Diff #68130)

Do we have a noduplicate for this case?

243–244 ↗(On Diff #68130)

Is this the right message?

263 ↗(On Diff #68130)

Maybe a comment here?

264 ↗(On Diff #68130)

I'd sink this into the if.

264 ↗(On Diff #68130)

auto *.

273–277 ↗(On Diff #68130)

I'd just call changeToUnreachable here.

  • Mark final suspend and fallthrough coro end as no duplicate in coro early
  • Remove unused functions from CoroFrame (but they are coming back in the next patch with vengeance)
  • Use changeToUnreachable to simplify code in CoroSplit and Coroutines.cpp
  • Bring more comments in compliance with coding style
GorNishanov marked 16 inline comments as done.Aug 16 2016, 10:10 AM
GorNishanov added inline comments.
lib/Transforms/Coroutines/CoroSplit.cpp
156 ↗(On Diff #68130)

Added a comment: The buildCoroutineFrame algorithm already rewritten access to the args that occurs after suspend points with loads and stores to/from the coroutine frame.

lib/Transforms/Coroutines/Coroutines.cpp
215–217 ↗(On Diff #68130)

We do now. I added code to CoroEarly to mark them no duplicate

GorNishanov marked 2 inline comments as done.

add /*UseLLVMTrap=*/ to a bool parameter

majnemer accepted this revision.Aug 16 2016, 10:19 AM
majnemer edited edge metadata.

LGTM with nits :)

lib/Transforms/Coroutines/CoroFrame.cpp
93 ↗(On Diff #68210)

Period.

100 ↗(On Diff #68210)

Ditto.

lib/Transforms/Coroutines/CoroInternal.h
90 ↗(On Diff #68210)

This comment is not very useful, I'd remove it.

lib/Transforms/Coroutines/Coroutines.cpp
186–187 ↗(On Diff #68210)

auto *

This revision is now accepted and ready to land.Aug 16 2016, 10:19 AM
GorNishanov edited edge metadata.

Nits addressed. Preparing to land

This revision was automatically updated to reflect the committed changes.