Page MenuHomePhabricator

[Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle
ClosedPublic

Authored by lxfind on Sep 10 2020, 10:27 AM.

Details

Summary

In generating the code for symmetric transfer, a temporary object is created to store the returned handle from await_suspend() call of the awaiter. Previously this temp won't be cleaned up until very later, which ends up causing this temp to be spilled to the heap. However, we know that this temp will no longer be needed after we obtained the address. We can clean it up right after the call to address member.

Diff Detail

Event Timeline

lxfind created this revision.Sep 10 2020, 10:27 AM
lxfind requested review of this revision.Sep 10 2020, 10:27 AM

Thanks for the change. LGTM, and testcase?

Thanks for the change. LGTM, and testcase?

Not sure how to add a test case for this though. We don't seem to explicitly test AST output.

Also LGTM with a testcase. It's fine to test the result of IRGen.

lxfind updated this revision to Diff 291140.Sep 10 2020, 11:37 PM

Add test case. Verify that the size of the frame reduced by 8.

hmm @rjmccall, I don't think there is a stable way to test this. The code generated for symmetric transfer is way too complicated to stably pattern match one less item in the frame.

lxfind updated this revision to Diff 291255.Sep 11 2020, 9:51 AM

Remove unstable test

hmm @rjmccall, I don't think there is a stable way to test this. The code generated for symmetric transfer is way too complicated to stably pattern match one less item in the frame.

I don't know that much about the code-generation here, but when cleanups escape into a surrounding scope, you can usually test that the cleanup is emitted before some call tied to a later statement in that scope. You don't have to test for every last instruction in the function.

lxfind updated this revision to Diff 291321.Sep 11 2020, 1:17 PM

Add test to verify that lifetime.end appears right after the address call.

lxfind edited the summary of this revision. (Show Details)Sep 11 2020, 1:19 PM
lxfind updated this revision to Diff 291324.Sep 11 2020, 1:22 PM

remove asan option, not needed

rjmccall accepted this revision.Sep 11 2020, 1:31 PM

Thanks, LGTM.

This revision is now accepted and ready to land.Sep 11 2020, 1:31 PM

Thanks, LGTM.

Thank you for reviewing and the suggestions on testcase!

This revision was landed with ongoing or failed builds.Sep 11 2020, 1:36 PM
This revision was automatically updated to reflect the committed changes.

@lxfind , could you backport this to branch 11?

@lxfind , could you backport this to branch 11?

I am actually seeing some problems with this change. Still investigating.