Page MenuHomePhabricator

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

Authored by GorNishanov on Aug 11 2016, 10:54 AM.

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

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
925

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–122

We should probably say something about coro.id here.

426

Missing the token.

lib/Transforms/Coroutines/CoroEarly.cpp
55–60

Did you intend to send this part out for review?

lib/Transforms/Coroutines/CoroElide.cpp
117

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

164–166

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–60

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

lib/Transforms/Coroutines/CoroElide.cpp
117

Cool! I'll switch to that.

164–166

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.