Page MenuHomePhabricator

[Coroutines] CoroElide enhancement
ClosedPublic

Authored by junparser on Dec 18 2019, 7:07 AM.

Details

Summary

According to https://reviews.llvm.org/D43242, coroelide pass only considers llvm.coro.destroy that dominate non-exceptional return. However, this cannot be met if current function is coroutine function because of suspend point.
Consider case from http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0978r0.html :

task<> big_task() { // lifetime of `subtask` is fully enclosed in its caller
  co_await subtask(); // coroutine frame allocation elided 
}

such cases should be optimized.

this patch does two changes in shouldElide function:

  1. Consider final coro.suspend as real non-exceptional terminator when function is coroutine, since final suspend is executed with or without exception at last.
  2. When function body is wrapped by try-catch, block of coro.suspend may has multiple predecessors : one normal path and the others are exceptional path, and they do not dominate each other. If all the predecessors dominate coro.destroys that reference same coro.begin, we consider condition is met.
  3. Keep normal function as usual

Test Plan:
Compile https://godbolt.org/g/mCKfnr and confirm it is still optimized to a single instruction, 'return 1190'.
Compile and run https://godbolt.org/z/AbdRw4, there is no "new" and "delete" output
Compile and run https://godbolt.org/z/gjYSi5 , there is no "new" and "delete" output
check-llvm

Diff Detail

Event Timeline

junparser created this revision.Dec 18 2019, 7:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2019, 7:07 AM
junparser edited the summary of this revision. (Show Details)Dec 18 2019, 7:08 AM

@modocache kindly ping~

@modocache any comments?

gental ping~

modocache accepted this revision.Feb 23 2020, 7:45 AM

Sorry this review took so long, and thank you for addressing the regression I introduced in D43242!

This patch no longer cleanly applies due to my changes in D71900, but I believe I was able to rebase it correctly in P8200. Based on that I think this looks like a great improvement, thank you!

Do you have commit access now, or would you like me to commit this (once you've updated it to apply cleanly) on your behalf?

llvm/lib/Transforms/Coroutines/CoroElide.cpp
161

typo: "coroutine". Also, nit-pick comment, but: I would write this "Consider the final coro.suspend as the real terminator when the current function is a coroutine." But I might be misunderstanding what you mean by "real terminator" and "coroutine expanded function" here.

This revision is now accepted and ready to land.Feb 23 2020, 7:45 AM

Sorry this review took so long, and thank you for addressing the regression I introduced in D43242!

This patch no longer cleanly applies due to my changes in D71900, but I believe I was able to rebase it correctly in P8200. Based on that I think this looks like a great improvement, thank you!

Do you have commit access now, or would you like me to commit this (once you've updated it to apply cleanly) on your behalf?

Yes , thanks for review the patch.
And I have got the commit access, however, how do I push the patch? Any hint for first time checkin ? Thank you again.

junparser updated this revision to Diff 247158.Feb 27 2020, 6:39 PM

Rebase and update the patch as suggestion.

junparser added a comment.EditedFeb 27 2020, 6:43 PM

Sorry this review took so long, and thank you for addressing the regression I introduced in D43242!

This patch no longer cleanly applies due to my changes in D71900, but I believe I was able to rebase it correctly in P8200. Based on that I think this looks like a great improvement, thank you!

Do you have commit access now, or would you like me to commit this (once you've updated it to apply cleanly) on your behalf?

Yes , thanks for review the patch.
And I have got the commit access, however, how do I push the patch? Any hint for first time checkin ? Thank you again.

@modocache Thanks, I have committed the patch.

This revision was automatically updated to reflect the committed changes.