Page MenuHomePhabricator

[coroutines] Part 4a: Coroutine Devirtualization: Lower coro.resume and coro.destroy.
ClosedPublic

Authored by GorNishanov on Jul 31 2016, 5:21 AM.

Details

Summary

This is the forth patch in the coroutine series. CoroEaly pass now lowers coro.resume
and coro.destroy intrinsics by replacing them with an indirect call to an address
returned by coro.subfn.addr intrinsic. This is done so that CGPassManager recognizes
devirtualization when CoroElide replaces a call to coro.subfn.addr with an appropriate
function address.

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 <= we are here
c) Do devirtualization

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 retitled this revision from to [coroutines] Part 4a: Coroutine Devirtualization: Lower 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.Jul 31 2016, 8:15 AM
lib/Transforms/Coroutines/CoroEarly.cpp
28–31 ↗(On Diff #66241)

Please clang-format this, pointers and refs lean right.

41–42 ↗(On Diff #66241)

This doesn't look clang-format'd.

44 ↗(On Diff #66241)

Why do we change the calling convention?

47 ↗(On Diff #66241)

clang-format this, pointers and refs lean right.

48 ↗(On Diff #66241)

Variables start with caps.

51 ↗(On Diff #66241)

I'd suggest auto * to make it clear we are copying around a pointer.

70 ↗(On Diff #66241)

Please clang-format this.

71 ↗(On Diff #66241)

Could you use the llvm_coro_{destroy,resume} intrinsic IDs instead?

72 ↗(On Diff #66241)

LLVM has a make_unique implementation.

74 ↗(On Diff #66241)

Please clang-format this.

89 ↗(On Diff #66241)

The reference should lean right.

lib/Transforms/Coroutines/CoroInstr.h
1 ↗(On Diff #66241)

The file path doesn't match the file, I'd just make it "CoroInstr.h"

36 ↗(On Diff #66241)

We don't use this naming style for enums, see here

47 ↗(On Diff #66241)

I'd make this auto just int64_t.

53 ↗(On Diff #66241)

clang-format this.

lib/Transforms/Coroutines/CoroInternal.h
20–23 ↗(On Diff #66241)

Please sort these.

34–36 ↗(On Diff #66241)

Pointers lean right.

lib/Transforms/Coroutines/Coroutines.cpp
70–74 ↗(On Diff #66241)

Please clang-format this.

82–93 ↗(On Diff #66241)

This doesn't look clang-format'd.

84 ↗(On Diff #66241)

Should we assert that Index is >= 0?

84–92 ↗(On Diff #66241)

We prefer auto * to make it clear we are copying around pointers.

87–88 ↗(On Diff #66241)

I think you can just sink the args list right into the CallInst::Create, ArrayRef has a std::initializer_list constructor.

mehdi_amini added inline comments.Jul 31 2016, 11:55 AM
lib/Transforms/Coroutines/CoroEarly.cpp
71 ↗(On Diff #66241)

Can we find if an intrinsic is declared/used in a Module using the ID?

mehdi_amini added inline comments.Jul 31 2016, 12:00 PM
lib/Transforms/Coroutines/CoroInternal.h
35 ↗(On Diff #66241)

M and C aren't great variable names. Also, is C used outside of the ctor? (The name does not help for grep...)

majnemer added inline comments.Jul 31 2016, 12:36 PM
lib/Transforms/Coroutines/CoroEarly.cpp
71 ↗(On Diff #66241)

Ah, maybe not...

GorNishanov added inline comments.Aug 1 2016, 5:51 AM
lib/Transforms/Coroutines/CoroEarly.cpp
44 ↗(On Diff #66241)

The coro.resume / coro.destroy intrinsics hide from the frontend details of the exact calling convention of post split coroutine subfunctions. LLVM knows that the extracted parts will have fastcc calling conventions (since we will be generating them in CoroSplit and will select fastcc calling convention in later patches).

71 ↗(On Diff #66241)

Yep. M.getDeclaration(ID, Types) will call getOrInsertFunction that will insert a declaration if one was not available.

I was looking for least expensive check and was following an example of:

/// \brief Test if the given module looks interesting to run ARC optimization on
inline bool ModuleHasARC(const Module &M) {
  return
    M.getNamedValue("objc_retain") ||
    M.getNamedValue("objc_release") ||
lib/Transforms/Coroutines/CoroInstr.h
36 ↗(On Diff #66241)

s/kFrame/Frame
s/kIndex/Index ?

It is a private enum inside of the class, so it does not need a prefix according to the coding guidelines.

I did want to have it stand out a little to signify that it is a constant. I considered

s/kFrame/FRAME
s/kIndex/INDEX ?

but then it maybe consumed with a macro.

lib/Transforms/Coroutines/CoroInternal.h
35 ↗(On Diff #66241)

I was following examples of similar classes in other components, such as in FunctionImportUtils.h, WholeProgramDevirt.cpp and many others.

But, I will be happy to change them to:

LLVMContext &Context;
Module &TheModule;

These also seem to be popular choices in the codebase.

Context is not used outside of the constructor in this patch. It will be in future patches. I can remove it for now.

majnemer added inline comments.Aug 1 2016, 9:55 AM
lib/Transforms/Coroutines/CoroInstr.h
36 ↗(On Diff #66241)

I'd name it Frame and Index.

lib/Transforms/Coroutines/Coroutines.cpp
82–83 ↗(On Diff #66241)

Please clang-format this.

mehdi_amini added inline comments.Aug 1 2016, 9:57 AM
lib/Transforms/Coroutines/CoroInternal.h
35 ↗(On Diff #66241)

Yeah, maybe it's me, but I keep single letter variable name for an iterator or other "short-lived" ones.

If you already know for sure you're gonna need soon the Context in multiple methods of this class, I don't mind you leaving it here for now.

GorNishanov updated this revision to Diff 66479.Aug 2 2016, 8:13 AM
GorNishanov removed rL LLVM as the repository for this revision.

Addressed review comments:

  1. Wholesale clang-format
  2. M/C renamed TheModule/Context
  3. kIndex, kFrame renamed IndexArg, FrameArg
  4. use {Frame,Index} instead of creating a SmallVector<2, Arg>
  5. auto X / auto* X where appropriate
  6. Continue using getNamedValue as a way to check if there is any work to do.
  7. added assert in makeSubFnCall to check that index is in range
  8. use llvm::make_unique
  9. s/changed/Changed
GorNishanov marked 31 inline comments as done.Aug 2 2016, 8:16 AM

Checked all checkboxes as done.

majnemer accepted this revision.Aug 2 2016, 9:15 AM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 2 2016, 9:15 AM
GorNishanov updated this revision to Diff 66702.Aug 3 2016, 1:19 PM
GorNishanov updated this object.
GorNishanov edited edge metadata.

Folded part4b (handling EH for coro.resume / coro.destroy) into this patch.

mehdi_amini added inline comments.Aug 3 2016, 1:30 PM
lib/Transforms/Coroutines/CoroInstr.h
32 ↗(On Diff #66702)

It is not great to use #define in headers, especially public ones.

GorNishanov updated this revision to Diff 66708.Aug 3 2016, 1:59 PM

Update CallSite::getIntrinsicID(), not to use anything from Intrinsics.h

GorNishanov added inline comments.Aug 3 2016, 2:03 PM
lib/Transforms/Coroutines/CoroInstr.h
33 ↗(On Diff #66708)

It is internal header used by Coroutine library. These constants are used (or will be used) by several coroutine passes in this directory.

When we drop MSVC 2013, these #define could be replaced with constexpr StringRef or something like that.

mehdi_amini added inline comments.Aug 3 2016, 8:39 PM
lib/Transforms/Coroutines/CoroInstr.h
33 ↗(On Diff #66708)

Sorry I looked at the path and for some reason thought it was a public one...

Nevertheless, I wonder if other passes are even trying to "hide" the intrinsic names or just using it plain everywhere, I might more readable to read M.getNamedValue("llvm.coro.destroy") instead of M.getNamedValue(CORO_DESTROY_STR)

GorNishanov updated this revision to Diff 66787.Aug 4 2016, 4:53 AM

Switch to using

declaresIntrinsics(M, {"llvm.coro.resume", "llvm.coro.destroy"})

helper function that validates that there is a named value in the module and that the name is one of the coroutine intrinsics (in debug mode).

GorNishanov updated this revision to Diff 66789.Aug 4 2016, 4:56 AM

remove CORO_DESTROY_STR and CORO_RESUME_STR macros, as now we can use declaresIntrinsics helper function.

GorNishanov added inline comments.Aug 4 2016, 4:59 AM
lib/Transforms/Coroutines/CoroInstr.h
34 ↗(On Diff #66789)

How about now? I got rid of the macros and use plain strings in a call to declaresIntrinsic helper function.

To guard against mis-spelling, delaresIntrinsic in debug mode checks whether the name is a valid coroutine intrinsic name.

add getIntrinsicName() to CoroXXXInst wrappers and use it instead of #define CORO_XXX_STR

majnemer added inline comments.Aug 4 2016, 12:44 PM
include/llvm/IR/CallSite.h
116 ↗(On Diff #66841)

Pointers lean right.

lib/Transforms/Coroutines/CoroInstr.h
32 ↗(On Diff #66841)

Hmm, this looks like trouble. CoroResumeInst is an IntrinsicInst but llvm.coro.resume is marked as Throws and IntrinsicInst derives from CallInst.

I think the right thing to do here is to make a helper function... Something like getIntrinsicCallSite which takes the intrinsic id (maybe as a template parameter? maybe not?) and returns an empty CallSite if there is no match or a populated CallSite if there was a match.

45 ↗(On Diff #66841)

Ditto.

GorNishanov updated this revision to Diff 66846.Aug 4 2016, 1:05 PM

Go back to declaresIntrinsic and fix "auto* F" to "auto *F" in CallSite.h

GorNishanov marked an inline comment as done.Aug 4 2016, 1:06 PM

LGTM w/ nits addressed.

lib/Transforms/Coroutines/Coroutines.cpp
99 ↗(On Diff #66846)

Functions start with a lower case letter.

122–124 ↗(On Diff #66846)

Please clang-format this.

GorNishanov updated this revision to Diff 66851.Aug 4 2016, 1:17 PM

Nits addressed

GorNishanov marked 2 inline comments as done.Aug 4 2016, 1:18 PM
This revision was automatically updated to reflect the committed changes.