This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines]: Part6b: Add coro.id intrinsic.
ClosedPublic

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

Details

Summary
  1. Make coroutine representation more robust against optimization that may duplicate instruction by introducing coro.id intrinsics that returns a token that will get fed into coro.alloc and coro.begin. Due to coro.id returning a token, it won't get duplicated and can be used as reliable indicator of coroutine identify when a particular coroutine call gets inlined.
  2. Move last three arguments of coro.begin into coro.id as they will be shared if coro.begin will get duplicated.
  3. doc + test + code updated to support the new intrinsic.

Diff Detail

Repository
rL LLVM

Event Timeline

GorNishanov retitled this revision from to Coroutine: part6 preview.
GorNishanov updated this object.
GorNishanov added reviewers: mehdi_amini, majnemer.
majnemer accepted this revision.Aug 11 2016, 12:00 PM
majnemer edited edge metadata.

LGTM with nits addressed

docs/Coroutines.rst
937 ↗(On Diff #67709)

optimization

This revision is now accepted and ready to land.Aug 11 2016, 12:00 PM
majnemer edited edge metadata.Aug 11 2016, 12:32 PM
majnemer added a subscriber: llvm-commits.
GorNishanov retitled this revision from Coroutine: part6 preview to [Coroutines]: Part6b: Add coro.id intrinsic..
GorNishanov updated this object.
GorNishanov requested a review of this revision.Aug 11 2016, 9:27 PM
GorNishanov edited edge metadata.

A lot of churn, needs a review.

GorNishanov edited edge metadata.

s/optimizatoin/optimization

majnemer added inline comments.Aug 11 2016, 9:49 PM
docs/Coroutines.rst
117–121 ↗(On Diff #67796)

We should probably say something about coro.id here.

426 ↗(On Diff #67796)

Missing the token.

lib/Transforms/Coroutines/CoroEarly.cpp
55–58 ↗(On Diff #67796)

Did you intend to send this part out for review?

lib/Transforms/Coroutines/CoroElide.cpp
117 ↗(On Diff #67796)

This could just be auto *False = ConstantInt::getFalse(C)

164–166 ↗(On Diff #67796)

Hmm... Is it possible for a coro.subfn to use a coro.begin via something weird like a phi?

GorNishanov marked 2 inline comments as done.Aug 11 2016, 9:52 PM
GorNishanov added inline comments.
lib/Transforms/Coroutines/CoroEarly.cpp
55–58 ↗(On Diff #67796)

No. I'll take it out. It is for the next patch.

lib/Transforms/Coroutines/CoroElide.cpp
117 ↗(On Diff #67796)

Cool! I'll switch to that.

164–166 ↗(On Diff #67796)

Sure, but, in that case, it would mean that the handle to coroutine escaped, moved out, was zero out and thus is not eligible for devirtualization.

GorNishanov marked 2 inline comments as done.

Feedback addressed. Preparing for landing.

  1. Fixes in Coroutines.rst
  2. use ConstantInt::getFalse
  3. remove commented out code
  4. add clarification comment about coro.subfn.addr
GorNishanov marked 6 inline comments as done.Aug 11 2016, 10:19 PM
This revision was automatically updated to reflect the committed changes.