Page MenuHomePhabricator

[coroutines] Add codegen for await and yield expressions
ClosedPublic

Authored by GorNishanov on Mar 9 2017, 9:37 PM.

Details

Summary

Emit suspend expression which roughly looks like:

auto && x = CommonExpr();
if (!x.await_ready()) {
   llvm_coro_save();
   x.await_suspend(...);     (*)
   llvm_coro_suspend(); (**)
}
x.await_resume();

where the result of the entire expression is the result of x.await_resume()

(*) If x.await_suspend return type is bool, it allows to veto a suspend:
if (x.await_suspend(...)) 
   llvm_coro_suspend();

(**) llvm_coro_suspend() encodes three possible continuations as a switch instruction:

%where-to = call i8 @llvm.coro.suspend(...)
switch i8 %where-to, label %coro.ret [ ; jump to epilogue to suspend
  i8 0, label %yield.ready   ; go here when resumed
  i8 1, label %yield.cleanup ; go here when destroyed
]

Diff Detail

Event Timeline

GorNishanov created this revision.Mar 9 2017, 9:37 PM

+ @majnemer who is most familiar with LLVM Coroutines representation.

majnemer added inline comments.Mar 11 2017, 3:51 PM
lib/CodeGen/CGCoroutine.cpp
24–28

Shouldn't this struct be in an anonymous namespace?

26

I'd move this into buildSuspendSuffixStr.

95–97

I'd just make this fully covered, it's just two more cases.

lib/CodeGen/CGExprScalar.cpp
279–282

Pointers should lean right.

285

Formatting looks off.

Addressed review feedback

  • clang-format three functions in CGExprScalar.cpp
  • got rid of AwaitKindStr array and select appropriate string in the switch statement in buildAwaitKindPrefix
GorNishanov marked 4 inline comments as done.Mar 11 2017, 6:56 PM
GorNishanov added inline comments.
lib/CodeGen/CGCoroutine.cpp
24–28

It is forward declared in CodeGenFunction.h and CodeGenFunction class holds a unique_ptr to CGCoroData as a member.
Here is the snippet from CodeGenFunction.h:

// Holds coroutine data if the current function is a coroutine. We use a
// wrapper to manage its lifetime, so that we don't have to define CGCoroData
// in this header.
struct CGCoroInfo {
  std::unique_ptr<CGCoroData> Data;
  CGCoroInfo();
  ~CGCoroInfo();
};
CGCoroInfo CurCoro;
majnemer added inline comments.Mar 11 2017, 7:11 PM
lib/CodeGen/CGCoroutine.cpp
95

I'd use a StringRef here.

Implemented review feedback: const char * => StringRef in buildSuspendSuffixStr

GorNishanov marked an inline comment as done.Mar 12 2017, 8:38 AM
GorNishanov added inline comments.
lib/CodeGen/CGCoroutine.cpp
95

StringRef it is.

GorNishanov marked 2 inline comments as done.Mar 12 2017, 8:50 AM
majnemer added inline comments.Mar 13 2017, 4:24 PM
lib/CodeGen/CGCoroutine.cpp
95

I'd just let the default constructor do its thing.

rjmccall added inline comments.Mar 14 2017, 12:01 AM
lib/CodeGen/CGCoroutine.cpp
24–28

If it's already forward-declared, you should check here that you're matching it by doing:

struct CodeGen::CGCoroData {
  ...
};

instead of making namespace declarations.

40

Comments about what these mean would be useful.

95

Agreed. Assigning 0 to a StringRef reads very wrong.

213

This function should return an RValue and take an AggValueSlot, just like every other generically-typed expression emitter. That way, you can just use EmitAnyExpr here instead of inlining two of three cases.

And you should implement it in CGExprComplex, of course, and add that as a test case.

  • reworked EmitAwait/Yield to has the signature similar to EmitAnyExpr
  • await expresions now support _Complex types.
  • s/Suffix/Prefix in buildCoroutineSuffix, since it was actually building a prefix
  • buildCoroutineSuffix uses literal array for names, as opposed to getting names from the switch stmt.
  • added tests to test for Aggr, Scalar and Complex
  • added tests for codegen for operator co_await
  • sprinkled more comments

Thank you very much for your feedback, John and David!

GorNishanov marked 4 inline comments as done.Mar 14 2017, 4:27 PM
GorNishanov added inline comments.
lib/CodeGen/CGCoroutine.cpp
26

I prefer to keep AwaitKindStr here, so it is trivial to visually inspect that the order in the array matches the order of the enum.

95

I got rid of this one and now use a llvm::StringLiteral array to get names.

213

Thank you! That was very helpful. The code shrunk and looks nicer now. Not sure, if ignoreResult is needed, but, I added it as well to match EmitAnyExpr.

GorNishanov marked 2 inline comments as done.Mar 20 2017, 9:01 AM

Looks good now?

rjmccall accepted this revision.Mar 25 2017, 11:29 AM

Looks great, thanks.

This revision is now accepted and ready to land.Mar 25 2017, 11:29 AM

Thank you very much for the review!

Merged with the top of the trunk and preparing to land

GorNishanov closed this revision.Mar 26 2017, 6:23 AM

Commit: r298784