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

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

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

44

auto *

44

Should this be a CallSite instead?

lib/Transforms/Coroutines/CoroEarly.cpp
52

Pointers lean right.

lib/Transforms/Coroutines/CoroFrame.cpp
30

auto *

31

You could use &BB->front().

47

Please have a period at the end of this comment.

64–67

auto *

69

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

102

auto *

110

auto *

lib/Transforms/Coroutines/Coroutines.cpp
197

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

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

44

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

lib/Transforms/Coroutines/CoroFrame.cpp
64–67

Missed those. Will upload in a sec.

lib/Transforms/Coroutines/Coroutines.cpp
197

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
23

Do we still need this include?

105

auto *

lib/Transforms/Coroutines/CoroSplit.cpp
156

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

lib/Transforms/Coroutines/Coroutines.cpp
205–207

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.

210

Ditto.

227–228

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
27–45

These functions seem dead.

lib/Transforms/Coroutines/CoroSplit.cpp
132

Caps + period.

156

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

172

Period.

174–175

You could use changeToUnreachable.

185

Ditto.

271

Args.empty().

281

Period.

313

Caps + period.

lib/Transforms/Coroutines/Coroutines.cpp
216–218

Do we have a noduplicate for this case?

244–245

Is this the right message?

264

Maybe a comment here?

265

I'd sink this into the if.

265

auto *.

274–278

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

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
216–218

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

Period.

100

Ditto.

lib/Transforms/Coroutines/CoroInternal.h
90

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

lib/Transforms/Coroutines/Coroutines.cpp
186–187

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.