Page MenuHomePhabricator

[coroutines] Improve suspend point simplification

Authored by GorNishanov on Nov 30 2018, 10:18 PM.



Enable suspend point simplification for cases where:

  • and coro.suspend are in different basic blocks
  • where there are intervening intrinsics

Diff Detail


Event Timeline

GorNishanov created this revision.Nov 30 2018, 10:18 PM
modocache requested changes to this revision.Dec 10 2018, 6:41 PM

Sorry for the wait! Mostly nits, but also I think the while condition might be inverted? It seems like, as is, it would never execute the body of the loop...?

544 ↗(On Diff #176246)

nit: Run clang-format on this; I believe it should be Instruction *From, but in any case From and To should be consistent.

566 ↗(On Diff #176246)

Is this condition inverted because of a typo? I'd expect this to be while (!Worklist.empty()) { ... }.

62 ↗(On Diff #176246)

nit: Just some typos, should be: "should not prevent simplification" -- note the extra "be" and the missing "p".

This revision now requires changes to proceed.Dec 10 2018, 6:41 PM

Implemented review feedback (and fix the test that missed the bug discovered by CR)

GorNishanov marked 4 inline comments as done.Dec 10 2018, 9:04 PM
GorNishanov added inline comments.
566 ↗(On Diff #176246)

Good catch. My tests were not catching it because they were too blunt, running entire -O2 pipeline as opposed to just coro-split pass.

Now, tests are running just coro-split pass and caught this mistake too which is fixed now.

Thank you for catching it!

modocache accepted this revision.Dec 11 2018, 7:54 AM

LGTM! As it happens I was reading this code the other day and wondering why it wasn't able to eliminate a suspend point I'd been looking at. I'll try it again and see if this new behavior is able to eliminate -- thanks!

This revision is now accepted and ready to land.Dec 11 2018, 7:54 AM
GorNishanov marked an inline comment as done.

small test tweak. Preparing to land

This revision was automatically updated to reflect the committed changes.