This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Refactor sinkLifetimeStartMarkers
ClosedPublic

Authored by junparser on Jul 8 2020, 3:43 AM.

Details

Summary

D82314 has implemented a method for coroutine frame reducing in sinkLifetimeStartMarkers, which sink lifetime.start marker to multiple users. However, this may cause mismatch. Consider this :

coroutine  A(){ 
  a; 
  lifetime.start(a);
  if (b) 
    {
      ... // do something
      co_await
      use(a);
    }
     use(a);
    lifetime.end(a);
}

Since both user of a do not dominate each other, we just sink lifetime.start for both of them. This may cause buildCoroutineFrame to keep a as local variable. More importantly, it introduces the pattern in resume function such like:

lifetime.start(a);
use(a);
lifetime.start(a);
use(a);
lifetime.end(a);

which cause wrong optimization in later passes.

This patch rewrite sinkLifetimeStartMarkers which only sink lifetime.start markers when all of its user are only used inside one of suspended region.

TestPlan: cppcoro, check-llvm, https://godbolt.org/z/zVx_eB

Diff Detail

Event Timeline

junparser created this revision.Jul 8 2020, 3:43 AM
lxfind added a comment.Jul 8 2020, 9:16 AM

Thank you for looking into the fix!

llvm/lib/Transforms/Coroutines/CoroFrame.cpp
1578

If I is a BitCastInst, wouldn't it be used by both lifetime.start and lifetime.end intrinsics, and hence has more than one user?

junparser marked an inline comment as done.Jul 8 2020, 7:31 PM
junparser added inline comments.
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
1578

Since sinkLifetimeStartMarkers is called after rewriteMaterializableInstructions, so if BitCastInst both used by lifetime.start and lifetime.end, then I should not cross the suspend point. So I believe that we can use hasOneUse here

lxfind accepted this revision.Jul 8 2020, 11:35 PM

Thank you!

This revision is now accepted and ready to land.Jul 8 2020, 11:35 PM
This revision was automatically updated to reflect the committed changes.