This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Part 9: Add cleanup subfunction.
ClosedPublic

Authored by GorNishanov on Aug 24 2016, 11:09 AM.

Details

Summary

[Coroutines] Part 9: Add cleanup subfunction.

This patch completes coroutine heap allocation elision. Now, the heap elision example from docs\Coroutines.rst compiles and produces expected result (see test/Transform/Coroutines/ex3.ll)

Intrinsic Changes:

  • coro.free gets a token parameter tying it to coro.id to allow reliably discovering all coro.frees associated with a particular coroutine.
  • coro.id gets an extra parameter that points back to a coroutine function. This allows to check whether a coro.id describes the enclosing function or it belongs to a different function that was later inlined.

CoroSplit now creates three subfunctions:

  1. f$resume - resume logic
  2. f$destroy - cleanup logic, followed by a deallocation code
  3. f$cleanup - just the cleanup code

CoroElide pass during devirtualization replaces coro.destroy with either f$destroy or f$cleanup depending whether heap elision is performed or not.

Other fixes, improvements:

  • Fixed buglet in Shape::buildFrame that was not creating coro.save properly if coroutine has more than one suspend point.
  • Switched to using variable width suspend index field (no longer limited to 32 bit index field can be as little as i1 or as large as i<whatever-size_t-is>)

Diff Detail

Repository
rL LLVM

Event Timeline

GorNishanov retitled this revision from to [Coroutines] Part 9: Add cleanup subfunction..
GorNishanov updated this object.
GorNishanov added a reviewer: majnemer.
GorNishanov added a subscriber: llvm-commits.
majnemer added inline comments.Aug 27 2016, 9:33 PM
docs/Coroutines.rst
806–810 ↗(On Diff #69140)

Hmm, coro.begin also takes the id. coro.free claims it reads the frame parameter, would anything break if we stopped doing that?

934–935 ↗(On Diff #69140)

This is a little strange...

lib/Transforms/Coroutines/CoroElide.cpp
150–158 ↗(On Diff #69140)

This looks like a duplicated version of Instruction::mayThrow

172–173 ↗(On Diff #69140)

Terminators are not the only instruction which may throw. Calls may throw as well.

GorNishanov added inline comments.Aug 28 2016, 5:57 PM
docs/Coroutines.rst
806–810 ↗(On Diff #69140)

I was thinking that if an allocation code gets unswitched we may get more than one coro.begin for the same coro.id and we would need to find all coro.begins and tied them back with a PHINode and use it as a replacement value for coro.free if heap elision is not performed.

Having it as a parameter allows LLVM to do it automatically for us.

934–935 ↗(On Diff #69140)

It was one of the ways I thought of how we can detect whether a coro.id we are looking at describes the enclosing function or it describes a different function due to inline coroutine start function into its caller.

This used to be important in the design where I was stroring "elide-or-not" boolean in the coroutine frame. Since now we switched to separate f$destroy and f$cleanup, identifying whether coro.id corresponds to the current function or not is no longer important.

I can take that parameter out altogether.

lib/Transforms/Coroutines/CoroElide.cpp
150–158 ↗(On Diff #69140)

Cool. I can inline this function into shouldElide as:

isa<ReturnInst> || mayThrow(T)

172–173 ↗(On Diff #69140)

After we talked on IRC, I am going to redo the shouldElide into something simpler. Something like:

If every coro.destroy points at coro.begin (that takes care of the escape, since if there is no direct SSA to coro.begin), it did escape and we were not able to put coro.begin in the vreg
<gor> majnemer: More precisely, if for every coro.begin I found a coro.destroy directly pointing at it, we are good for elision

Simplify shouldElide() to just check that for every coro.begin there is a coro.destroy referencing it directly.

GorNishanov marked 8 inline comments as done.Aug 28 2016, 8:56 PM
GorNishanov added inline comments.
docs/Coroutines.rst
934–935 ↗(On Diff #69140)

I'll keep if for now. We may run into another case where we will need it.

lib/Transforms/Coroutines/CoroElide.cpp
150–158 ↗(On Diff #69140)

This code was simplified and no longer require these.

majnemer accepted this revision.Aug 28 2016, 10:05 PM
majnemer edited edge metadata.

LGTM

lib/Transforms/Coroutines/CoroElide.cpp
143–156 ↗(On Diff #69527)

This seems dead.

171 ↗(On Diff #69527)

Pointers lean right.

This revision is now accepted and ready to land.Aug 28 2016, 10:05 PM
GorNishanov edited edge metadata.
GorNishanov marked 2 inline comments as done.

Nits addressed. Preparing to land.

GorNishanov marked 2 inline comments as done.Aug 29 2016, 6:56 AM
This revision was automatically updated to reflect the committed changes.