Page MenuHomePhabricator

[Coroutines] Fix PR45130
ClosedPublic

Authored by junparser on Mar 17 2020, 10:54 PM.

Details

Summary

For now, when final suspend can be simplified by simplifySuspendPoint, handleFinalSuspend is executed as well to remove last case in switch instruction. This causes issue in https://bugs.llvm.org/show_bug.cgi?id=45130, this patch fixes it.

TestPlan: check-llvm

Diff Detail

Event Timeline

junparser created this revision.Mar 17 2020, 10:54 PM
modocache accepted this revision.Mar 19 2020, 7:34 PM

Excellent, thank you! I had one question, otherwise this looks great!

llvm/lib/Transforms/Coroutines/CoroSplit.cpp
1161

Beginner question of mine: what is this assert testing? I would guess that it's testing that SI != nullptr, but the cast<CoroSuspendInst>(...) above would also fail were that the case. As far as I know CoroSuspendInst and its class hierarchy don't implement a boolean conversion operator either... so if this is a null check, I think it's redundant and can be removed, but if it does something more, please let me know!

This revision is now accepted and ready to land.Mar 19 2020, 7:34 PM
junparser marked an inline comment as done.Mar 19 2020, 8:20 PM

Thanks for review! :)

llvm/lib/Transforms/Coroutines/CoroSplit.cpp
1161

Thanks! you are completely right. This redundant assert testing added here is just my unintentional behavior.

junparser updated this revision to Diff 251546.Mar 19 2020, 8:25 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2020, 8:51 PM