This is an archive of the discontinued LLVM Phabricator instance.

[coro] Async coroutines: Allow more than 3 arguments in the dispatch function
ClosedPublic

Authored by aschwaighofer on Nov 9 2020, 1:04 PM.

Details

Summary

We need to be able to call function pointers. Inline the dispatch
function.

rdar://70097093

Diff Detail

Event Timeline

aschwaighofer created this revision.Nov 9 2020, 1:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2020, 1:04 PM
aschwaighofer requested review of this revision.Nov 9 2020, 1:04 PM

I don't quite understand the need to inline here.

aschwaighofer added a comment.EditedNov 10 2020, 8:24 AM

Semantically, it is not needed. But without inlining here this helper dispatch function thunk will remain at O0 because inlining is not run again.

aschwaighofer added a comment.EditedNov 10 2020, 8:26 AM

I am worried about introducing unnecessary thunks that the debugger has to step through.

Also inline the context project function used by the resume function.

What is this function being inlined? We expect the frontend to emit a thunk that glues things together?

Yes, frontend is expected to emit a thunk that models the tail call to the function at the suspend point. This prevents code moving in between the tail call and return before coroutine splitting and allows for specifying how to perform the call (e.g pointer authentication).

define internal hidden void @__suspend_dispatch_3.1(i8* %0, %task* %1, %executor* %2, %context* %3)  {
entry:
  %4 = bitcast i8* %0 to void (%task*, %executor*, %context*)*
  tail call swiftcc void %4(%task* %1, %executor* %2, %context* %3)
  ret void
}

define linkonce_odr hidden i8* @__resume_project_context(i8* %0) {
entry:
  %1 = bitcast i8* %0 to i8**
  %2 = load i8*, i8** %1, align 8
  ret i8* %2
}

%resume_func = call i8* @llvm.coro.async.resume()
%24 = call { i8*, i8*, i8* } (i8*, i8*, ...) @llvm.coro.suspend.async(
  i8* %resume_func, 
  i8* bitcast (i8* (i8*)* @__resume_project_context to i8*), 
  i8* bitcast (void (i8*, %task*, %executor*, %context*)* @__suspend_dispatch_3.1 to i8*), 
  i8* %fun_ptr, %task* %0, %executor* %1, %context* %22)

Transfer the debug location from the suspend point to the functions called at and after the suspend point (context projection).

I see. This LGTM as an immediate fix, then.

I wonder if we shouldn't have a different IR design, though. Maybe we should lower directly to IR that uses a pattern more like the SIL get_unsafe_continuation pattern? i.e.

%0 = call token @llvm.coro.async.continuation()  // returns the continuation function pointer; must be used by exactly one suspend
... // somewhere in here we store the continuation into the child context
call void @llvm.coro.async.suspend(token %0)

I can't remember if we discussed and discarded this.

Oh, unfortunately llvm.coro.async.continuation wouldn't be able to just return the continuation function pointer; there would have to be a llvm.coro.async.get_continuation_function intrinsic. But we do something similar with the alloc intrinsics.

Use the function argument index instead of the function argument in
coro.id.async. This solves any spurious use issues.

Coerce the arguments of the tail call function at a suspend point. The LLVM
optimizer seems to drop casts leading to a vararg intrinsic.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 11 2020, 3:27 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
lxfind added a subscriber: lxfind.Nov 13 2020, 1:26 PM
lxfind added inline comments.
llvm/lib/Transforms/Coroutines/CoroSplit.cpp
686

InlineRes will be unused in a release build. Add (void)?