This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Remove corresponding phi values when apply simplifyTerminatorLeadingToRet
ClosedPublic

Authored by junparser on Dec 22 2019, 11:17 PM.

Details

Summary

In addMustTailToCoroResumes, we set musttail on those resume instructions that are followed by a ret instruction. This is done by simplifyTerminatorLeadingToRet which replace a sequence of branches leading to a ret with a clone of the ret.

However it forgets to remove corresponding PHI values that come from basic block of replaced branch, and may cause jumpthreading pass hangs (https://bugs.llvm.org/show_bug.cgi?id=43720)

This patch fix this issue

Test Plan:
cppcoro library with O3+flto
check-llvm

Diff Detail

Event Timeline

junparser created this revision.Dec 22 2019, 11:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2019, 11:17 PM
modocache accepted this revision.Dec 27 2019, 11:15 AM

Sorry for the wait here, @junparser! I wanted to send my own patches out before looking at this one 😛

This is really nice, thanks for the change! As per your test plan, I confirmed that with this patch, cppcoro issue https://github.com/lewissbaker/cppcoro/issues/109 no longer reproduces for me. The jump threading issue you describe must have been responsible for Clang hanging indefinitely when compiling that test file.

I had one nitpick related to your test, please address it or not at your own discretion. This patch LGTM.

llvm/test/Transforms/Coroutines/coro-split-musttail1.ll
100

One nitpick: this test includes a lot of attribute group annotations, like #3 and #5, but these attribute groups aren't defined. If I run opt -verify on this IR file, I get this, which defines 3 attribute groups: https://reviews.llvm.org/P8178

As far as I know there's no practical reason to remove these non-existent attribute group references, but as a personal preference, I'd prefer if you modified the test such that it either (a) doesn't use undefined attribute groups, or (b) defines the attribute groups it uses, for example by using the output from https://reviews.llvm.org/P8178. The reason I'd prefer this is because I think it makes the LLVM IR slightly more readable -- as a reader I won't go looking for a definition of #3 that doesn't exist.

This revision is now accepted and ready to land.Dec 27 2019, 11:15 AM

Sorry for the wait here, @junparser! I wanted to send my own patches out before looking at this one 😛

This is really nice, thanks for the change! As per your test plan, I confirmed that with this patch, cppcoro issue https://github.com/lewissbaker/cppcoro/issues/109 no longer reproduces for me. The jump threading issue you describe must have been responsible for Clang hanging indefinitely when compiling that test file.

I had one nitpick related to your test, please address it or not at your own discretion. This patch LGTM.

tks, I‘ll update the testcase.

junparser updated this revision to Diff 235529.Dec 29 2019, 6:41 PM

update the testcase, and @modocache can you commit this patch? thanks.

This revision was automatically updated to reflect the committed changes.

Done! Please excuse the wait. Have you considered getting commit access to LLVM? You won't have to wait on me in the future :) https://llvm.org/docs/DeveloperPolicy.html#new-contributors

Done! Please excuse the wait. Have you considered getting commit access to LLVM? You won't have to wait on me in the future :) https://llvm.org/docs/DeveloperPolicy.html#new-contributors

Thanks, I'll apply the access.