This is an archive of the discontinued LLVM Phabricator instance.

Part 4c: Coroutine Devirtualization: Devirtualize coro.resume and coro.destroy.
ClosedPublic

Authored by GorNishanov on Aug 5 2016, 3:09 PM.
Tokens
"Yellow Medal" token, awarded by GorNishanov.

Details

Summary

This is the 4c patch of the coroutine series. CoroElide pass now checks if PostSplit coro.begin
is referenced by coro.subfn.addr intrinsics. If so replace coro.subfn.addrs with an appropriate coroutine
subfunction associated with that coro.begin.

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)
3.Add empty coroutine passes. (https://reviews.llvm.org/D22847)
4.Add coroutine devirtualization + tests.
ab) Lower coro.resume and coro.destroy (https://reviews.llvm.org/D22998)
c) Do devirtualization <= we are here
5.Add CGSCC restart trigger + tests.
6.Add coroutine heap elision + tests.
7.Add the rest of the logic (split into more patches)

Diff Detail

Repository
rL LLVM

Event Timeline

GorNishanov updated this revision to Diff 67030.Aug 5 2016, 3:09 PM
GorNishanov retitled this revision from to Part 4c: Coroutine Devirtualization: Devirtualize coro.resume and coro.destroy..
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 5 2016, 3:43 PM
lib/Transforms/Coroutines/CoroElide.cpp
66–74 ↗(On Diff #67030)

This can do a lot of unnecessary work because there can be lots of users of the Constant.

Instead, could we use replaceAndRecursivelySimplify? It is quite powerful.

I imagine something like:

for (CoroSubFnInst *I : Users)
  replaceAndRecursivelySimplify(I, Value);
106–109 ↗(On Diff #67030)

Any reason why you need the constant folder here? I'd have used ConstantExpr::getExtractValue.

lib/Transforms/Coroutines/CoroInstr.h
105 ↗(On Diff #67030)

It seems like the natural thing to do would be to mirror the logic for OutlinedParts.

If you wish to cast<> here, we need a verifier check to ensure that the GlobalVariable's initializer is either a ConstantArray or a ConstantStruct.

GorNishanov updated this revision to Diff 67050.Aug 5 2016, 5:34 PM
GorNishanov removed rL LLVM as the repository for this revision.
  1. removed useless continue in CoroEarly
  2. coro-elide.ll no longer runs coro-early pass
  3. add coro.begin sanity tester to verifier.cpp
  4. switch to using replaceAndRecursivelySimplify
  5. switch to ConstantExpr::extractValue
GorNishanov marked 3 inline comments as done.Aug 5 2016, 5:35 PM
mehdi_amini added inline comments.
lib/Transforms/Coroutines/CoroInstr.h
76 ↗(On Diff #67030)

frontend I guess?

84 ↗(On Diff #67030)

Constant array? Array of constants? I'm not sure what you wanted here but "array constant" doesn't seem right to me.

GorNishanov updated this revision to Diff 67051.Aug 5 2016, 6:11 PM

Fixed typo + clarified comment in CoroInstr.h

GorNishanov marked 2 inline comments as done.Aug 5 2016, 6:12 PM
majnemer accepted this revision.Aug 5 2016, 6:20 PM
majnemer edited edge metadata.

LGTM w/ nits addressed.

lib/Transforms/Coroutines/CoroInstr.h
96–98 ↗(On Diff #67051)

Can this ever fail? I thought the verifier checks to make sure that we have a GlobalVariable operand.

102–103 ↗(On Diff #67051)

Could we have a ConstantStruct test case as well?

test/Transforms/Coroutines/coro-elide.ll
30 ↗(On Diff #67051)

I'd recommend ending all these function names with an open paren like so:

; CHECK-LABEL: @callResume(

This way we will not confuse FileCheck when we add @callResumeComplicated()

This revision is now accepted and ready to land.Aug 5 2016, 6:20 PM
GorNishanov updated this revision to Diff 67052.Aug 5 2016, 7:08 PM
GorNishanov edited edge metadata.

CHECK-LABEL tweak per David's suggestion.

GorNishanov marked 3 inline comments as done.Aug 5 2016, 7:13 PM
GorNishanov added inline comments.
lib/Transforms/Coroutines/CoroInstr.h
96–98 ↗(On Diff #67052)

Yes. There are three legal alternative that are described in the comment above and checked by the verifier.

i8* null
ConstantStruct
ConstantArray

When coro.begin comes fresh out of the frontend, info parameter is null.

102–103 ↗(On Diff #67052)

Sure. In one of the patches working on CoroSplit, there will be a test with ConstantStruct.

This revision was automatically updated to reflect the committed changes.
GorNishanov marked 3 inline comments as done.