This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Part 8: Coroutine Frame Building algorithm
ClosedPublic

Authored by GorNishanov on Aug 16 2016, 4:05 PM.

Details

Summary

This patch adds coroutine frame building algorithm. Now, simple coroutines such as ex0.ll and ex1.ll (first examples from docs\Coroutines.rst can be compiled).

Documentation and overview is here: http://llvm.org/docs/Coroutines.html.

Upstreaming sequence (rough plan)
1.Add documentation. (https://reviews.llvm.org/D22603)
2.Add coroutine intrinsics. (https://reviews.llvm.org/D22659)
...

  1. Split coroutine into subfunctions. (https://reviews.llvm.org/D23461)
  2. Coroutine Frame Building algorithm <= we are here
  3. Add f.cleanup subfunction.

10+. The rest of the logic

Diff Detail

Event Timeline

GorNishanov retitled this revision from to [Coroutines] Part 8: Coroutine Frame Building algorithm.
GorNishanov updated this object.
GorNishanov added a reviewer: majnemer.
GorNishanov set the repository for this revision to rL LLVM.
GorNishanov added a subscriber: llvm-commits.
majnemer added inline comments.Aug 23 2016, 11:05 AM
lib/Transforms/Coroutines/CoroFrame.cpp
116

Perhaps "We've rewritten" or "We rewrote".

440–443

I think this case be E.user()->replaceUsesOfWith(CurrentValue, CurrentReload);

455–457

Could this be ReplaceInstWithInst(P.first, G);?

559–561

I think replaceUsesOfWith could be used here.

majnemer added inline comments.Aug 23 2016, 12:44 PM
lib/Transforms/Coroutines/CoroFrame.cpp
402–403

Maybe:

if (CurrentValue == E.def())
  continue;

This will let you reduce the indentation.

479–480

What prevents you from visiting the same pred twice?

483–484

I think you could structure this like a normal for loop:

for (auto *PN = cast<PHINode>(&BB.front()); PN != nullptr; PN = dyn_cast<PHINode>(PN->getNextNode()))
593–594

The comment says "final" but you are accessing .front().

GorNishanov removed rL LLVM as the repository for this revision.

Feedback implemented

GorNishanov marked 7 inline comments as done.Aug 23 2016, 3:19 PM
GorNishanov added inline comments.
lib/Transforms/Coroutines/CoroFrame.cpp
33

remove 'has'

402–403

I cannot do that. We need to finish processing the reloads for the CurrentValue. Spills datastructure work like multiset:
<def, use1>
<def, use2>
<def, use3>
etc.
Thus, if (CurrentValue == E.def()), I don't have to generate a spill (i.e. store),
But I do have to load the value for every use.

479–480

Hmm... Need to think about it a little bit more.

483–484

PN cannot be null on the first entry. So, used do-while instead. Saves one compare :)

GorNishanov marked 5 inline comments as done.Aug 23 2016, 3:44 PM
GorNishanov added inline comments.
lib/Transforms/Coroutines/CoroFrame.cpp
479–480

I do not think there is a correctness issue here.
For the case like:

entry:
    ...
    br i1 %x label %a, label %a
    ...
a:
    phi [0 %entry], [0 %entry], [1 %loop]

With the code as is, we will create two identical edge blocks:

entry:
    ...
    br i1 %x label %a1, label %a2
    ...
a1:
    %a1.val = phi [0 %entry]
    br label %a
a2:
    %a2.val = phi [0 %entry]
    br label %a
a3:
    %a3.val = phi [1 %loop]
    br label %a
a:
    phi [%a1.val %a1], [%a2.val %a2],  [%a3.val %a3]

I added a TODO to tighten up PHI rewriting so that for identical predecessors we can share an edge block, so that the example above would look like:

entry:
    ...
    br i1 %x label %a1, label %a2
    ...
a1:
    %a1.val = phi [0 %entry]
    br label %a
a2:
    %a2.val = phi [1 %loop]
    br label %a
a:
    phi [%a1.val %a1], [%a2.val %a2]
GorNishanov marked 2 inline comments as done.Aug 23 2016, 3:45 PM
majnemer accepted this revision.Aug 23 2016, 8:30 PM
majnemer edited edge metadata.

LGTM with nits addressed.

lib/Transforms/Coroutines/CoroFrame.cpp
95–98

I wouldn't bother with these, they'll get removed by the linker in a release build anyway.

137–145

I'd mark these LLVM_DUMP_METHOD.

626–638

Can an EHPad be across a suspend?

This revision is now accepted and ready to land.Aug 23 2016, 8:30 PM
GorNishanov edited edge metadata.

Nits addressed. Preparing to land

GorNishanov marked 4 inline comments as done.Aug 23 2016, 8:59 PM
GorNishanov added inline comments.
lib/Transforms/Coroutines/CoroFrame.cpp
626–638

Not supported. I added a report_fatal_error.

GorNishanov marked an inline comment as done.

rebase

rebase again

remove unused variable

This revision was automatically updated to reflect the committed changes.