fix lint

fix test

add test

comments

In D87470#2273310, @junparser wrote:

@lxfind , could you backport this to branch 11?

In D87596#2272950, @ChuanqiXu wrote:

In D87596#2271942, @lxfind wrote:

Thank you for working on this optimization!

This type of optimizations is typically not enabled in O0, and we probably don't want to either. Should we gate it under the optimization level?

This optimization depends on lifetime markers, which would not be produced in O0. So this optimization won't be enabled in O0.

rebase

Thank you for working on this optimization!

fix test failures

In D87470#2268911, @rjmccall wrote:

Thanks, LGTM.

remove asan option, not needed

Add test to verify that lifetime.end appears right after the address call.

Remove unstable test

hmm @rjmccall, I don't think there is a stable way to test this. The code generated for symmetric transfer is way too complicated to stably pattern match one less item in the frame.

Add test case. Verify that the size of the frame reduced by 8.

In D87470#2267060, @junparser wrote:

Thanks for the change. LGTM, and testcase？

rebase before land

address typo

In D86859#2255833, @modimo wrote:

Looking at https://bugs.llvm.org/show_bug.cgi?id=36578:
Andrew Kelley 2019-08-11 09:00:59 PDT
I'm no longer subscribed to this bug report. I've come to the conclusion that LLVM's coroutine API is not worth using, and resorting to implementing coroutines directly in the frontend.

That's an option to look into for solutions here. Also official docs for coro.begin: https://llvm.org/docs/Coroutines.html#llvm-coro-begin-intrinsic

Having anything emitted before frame-setup is tricky because any operation there needs to be tied into DWARF unwind codes. Mirroring these operations as the current design is doing may be a better solution given that. The assertion will definitely be a bug farm as every corner gets tested.

address lint

By mistake

In D66230#2246830, @rjmccall wrote:

Yes, we at Apple had similar problems and basically had to revert this internally. The basic problem is that LLVM's alloca representation is not well-suited for coroutines — we really need a guarantee that there are no stack allocations live across the coro.begin, or at least none that stay alive until the first suspend. That would be easy with something like SIL's alloc_stack, but LLVM's desire to push local allocations into the entry block really makes it difficult to even talk about the appropriate restrictions here.

Maybe we could force all local allocations in coroutines to be done with the llvm.coro.alloca.* intrinsics that I added? We could then actually verify that no allocations are live across the coro.begin. That would require a fair bit of abstraction in Clang, though, and possibly in some other passes that introduce allocas, and it might severely block optimization unless we teach mem2reg how to handle those intrinsics.

@ChuanqiXu Thank you for taking a look!

Update description

Use a different approach

Add reviewers

@GorNishanov This fix seems problematic.
Consider this code:

%var = alloca i32
%1 = getelementptr .. %var; stays put
%f = call i8* @llvm.coro.begin
store ... %1

After some thought, I think this is not a good change. Many part of the system/toolchain expects each part to be 16-bit.

Address comments

In D79219#2201415, @phosek wrote:

In D79219#2201109, @lxfind wrote:

@phosek, Under this change, now when I build LLVM (with a basic config cmake -G Ninja --LLVM_ENABLE_PROJECTS=clang ../llvm), in file build_dir/lib/cmake/llvm/LLVMExports.cmake, I see this:

set_target_properties(LLVMSupport PROPERTIES
  INTERFACE_LINK_LIBRARIES "curses;m;ZLIB::ZLIB;LLVMDemangle"

This seems broken to me. Can you take a look?

This is correct. That target is provided by find_package(ZLIB). In LLVMConfig.cmake, we invoke find_package(ZLIB) when zlib support is enabled. If you're using LLVMExports.cmake, you'll need to invoke find_package(ZLIB) yourself.

@arsenm, we use the direct names everywhere else though (if you scan through the rest of this file). Anyway, I don't mind using it as long as it works properly. I have seen (and have seen others asking in the other commit as well), that ZLIB::ZLIB shows up in build_dir/lib/cmake/llvm/LLVMExports.cmake. That needs to be resolved.

@phosek, Under this change, now when I build LLVM (with a basic config cmake -G Ninja --LLVM_ENABLE_PROJECTS=clang ../llvm), in file build_dir/lib/cmake/llvm/LLVMExports.cmake, I see this:

set_target_properties(LLVMSupport PROPERTIES
  INTERFACE_LINK_LIBRARIES "curses;m;ZLIB::ZLIB;LLVMDemangle"

Thank you!

Thank you for looking into the fix!

In D82992#2126898, @MaskRay wrote:

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

In D82986#2126479, @thakis wrote:

That's not enough, you also need to add an rm to remove the stale .LL file still on disk, see my comment on your original change.

Fix up in https://reviews.llvm.org/D82986

In D82928#2126018, @fhahn wrote:

Looks like this causes a bunch of build bot failures, e.g http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/31465 b

It would be great if you could take a look.

In D82928#2126018, @fhahn wrote:

Looks like this causes a bunch of build bot failures, e.g http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/31465 b

It would be great if you could take a look.

In D82928#2126018, @fhahn wrote:

Looks like this causes a bunch of build bot failures, e.g http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/31465 b

It would be great if you could take a look.

Rebase

In D82314#2124662, @junparser wrote:

In D82314#2124661, @junparser wrote:

@lxfind This patch causes some mismatch when variable is used in both resume and destroy function. Besides, we should move this patch and the check in buildCoroutineFrame.

@lxfind, Would you try to fix this？ If you do not have time, then I'll try do this. Thanks

Rebase before landing

In D82314#2117543, @lewissbaker wrote:

There are still room for improvements, in particular, GCC has a 4K frame for this function.

I think GCC having a smaller coroutine frame is probably because it does not yet implement the allocation-elision optimisation which inlines the nested coroutine frame (which is 4K) into the parent coroutine frame.
I have not looked yet, but I suspect that you'll see within the get_big_object2() code it will perform another heap allocation for the get_big_object() frame.

Until we have more general support for async RVO, I think 8K is probably as good as we're going to be able to get (4K for this coroutine's promise and 4K for child coroutine-frame and its promise).