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
29

auto *

30

You could use &BB->front().

46

Please have a period at the end of this comment.

63–66

auto *

68

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

101

auto *

109

auto *

lib/Transforms/Coroutines/Coroutines.cpp
196

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
63–66

Missed those. Will upload in a sec.

lib/Transforms/Coroutines/Coroutines.cpp
196

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
172

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

lib/Transforms/Coroutines/Coroutines.cpp
204–206

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

Ditto.

226–227

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
148

Caps + period.

172

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

188

Period.

190–191

You could use changeToUnreachable.

201

Ditto.

287

Args.empty().

297

Period.

329

Caps + period.

lib/Transforms/Coroutines/Coroutines.cpp
215–217

Do we have a noduplicate for this case?

243–244

Is this the right message?

263

Maybe a comment here?

264

I'd sink this into the if.

264

auto *.

273–277

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
172

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

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
94

Period.

101

Ditto.

lib/Transforms/Coroutines/CoroInternal.h
90

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

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

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.