I also agree that we should try to keep the compiler simple and not support the complicated case.
It should be fairly straightforward for a codebase to update fully to use std instead of std::experimental (we have a large coroutine codebase as well).
Given that everyone is mostly supportive, I will accept the change.

Thank you for fixing this!

LGTM. Thanks!

The warning seems redundant to the one in D108696?

In D108696, the clang would lookup in std and std::experimental namespace. And this patch would only be checked in after D108696 get checked in. So I think it wouldn't affect users much.

Well, but if user code has "#include <experimental/coroutine>", would it still compile?

My guess is you might need to keep coro in both std and std::experimental for a while, to allow smooth transition.

I am not familiar with the process of when to move something out of experimental, but I do wonder how this is normally done so that people who uses coroutines can have a smooth migration?
I assume that this is going to be a breaking change that existing code using coroutine will need to be updated and no longer compatible with old versions.

Thanks for working on this!
A few questions:

• Is this primarily an optimization, or does it also fix a bug?
• Does LLVM mid-end not have a nothrow attribute analysis pass? Doing this manually in the front-end seems a bit fragile (concerned about missing something)
• Do we expect those isXXXNoThrow methods to be called else where? If not, is there a reason we choose to put them in 6 separate functions, instead of one function?

In D105877#2923257, @nikic wrote:

I noticed that this change had a measurable impact on O0 memory usage, which I wouldn't have expected (https://llvm-compile-time-tracker.com/compare.php?from=0f9e6451a836886f39137818c4f0cfd69ae31e62&to=8a1727ba51d262365b0d9fe10fef7e50da7022cd&stat=max-rss). Any idea what could cause it? Some additional analysis results hanging around?

Hi! I have a question regarding the implementation of "VisitMaterializeTemporaryExpr". Specifically, I wonder if we should skip visiting the children? Would't visiting the children of MaterializeTemporaryExpr cause the same expression to be visited twice?

Thank you!

Having both "--show-mmap-events --ignore-mmap-events" seems a bit counter-intuitive.
If we already preprocessed the mmap events, could we also assume that we have the right pids and hence don't need --show-mmap-events?

In D105606#2903644, @ChuanqiXu wrote:

In D105606#2903641, @lxfind wrote:

In D105606#2889219, @ChuanqiXu wrote:

gentle ping~

fix lint?

I am not sure if I understand your point correctly. The intent of the diff is going to record the coroutines got elided. There are options to dump informations to specific file like info-output-file in Timer.cpp. So I guess we can do similar things in coroutine.

In D105606#2889219, @ChuanqiXu wrote:

gentle ping~

I am OK with this. You may want to move those part of the checking code to a separate function for readability. (since this check is independent from lifetime sinking, we could also consider doing the check earlier for all corosuspends too.

LGTM

fix warning

Put the post-split ramp function back to the CGSCC worklist

If coroutine ramp function couldn't get inlined, it would disable coroutine elide optimization. Could you elaborate more on why do you want to do that?

In D95807#2849053, @aeubanks wrote:

this will run the function simplification pipeline twice on every single function when coroutines are enabled, I don't think that's the intention

I thought the intention was to do all the the re-adding of SCCs inside CoroSplit.cpp, including the SCC with the function that was split

Cool! fix test failure?

In D95807#2846358, @ChuanqiXu wrote:

note that we don't really need to run Inliner again on the ramp function after split

This isn't accurate. The inline may run again for ramp function after split and it's required by coro elide.

If there is an inlining opportunity, it should have happened pre-split, right? Is there any reason it didn't happen pre-split but only post-split?

It seems like that we don't need the attribute CORO_PRESPLIT_ATTR any more, do we? If yes, I think we should remove them.

It's still needed by the legacy pass manager. I don't want to break that yet.

After removing the legacy test command, I was finally able to update this patch. It's now ready for review. I will update the decription to reflect to the latest changes

Yeah, but it may be inlined after splitting, which could trigger coro elide.

In O0, there is no inliner pass (after CoroSplit), so inlining should never happen.

In D105066#2845958, @ChuanqiXu wrote:

On O0, it is possible to inline if the user marked the function with always_inline.
Since CoroElide is kind of optimization, it should be OK to skip in O0.
Out of curiosity, what's the reason that you want to remove it?

update tests

By the way, while you are at it, I was looking at some of the sink lifetime tests, and I was confused by this test:
https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-02.ll
It's supposed to test that we can sink the lifetime (and hence will not put the alloca on the frame). But looks like we are not able to sink it and the test is testing that it will be on the frame?

rebase

In D104895#2840172, @ChuanqiXu wrote:

Thanks for working on this!
If the two commands behaves the same, I prefer the old styles since it looks simpler.

I believe the plan is to have all tests to be in -passes style with new pm (I.e -passes is the preferred style)

I guess I am fine with solution 1 and see how far we can go with it.
In the long run, we should look at cases where this optimization (sinkLifetimeStartMarkers) is effective and figure out why the lifetime start markers are emitted early, so that we could improve the lifetime marker creation in the first place. Overall it still feels a bit strange to move lifetime markers around (a sign that the front-end didn't do the right thing)

In D104807#2836908, @modimo wrote:

What would an IR change look like to enforce this?

In D104807#2836836, @wenlei wrote:

The change looks good. But...

• I wonder how many more times we'll have to deal with this.. It indeed seems hard to enforce without IR change :(
• Are we hitting SJLJ path? I thought there's no usage of that for our codebase.

To be honest, none of the solutions seems satisfying but I couldn't propose a better one.

Add test case for noalias argument

Abandon this in favor of D104184

update comment

I updated the description to reflex the summary of discussions from previous patches. Based on the discussion, we believe that this should be the right approach

In D104007#2817612, @efriedma wrote:

However latter optimization passes could very well optimize out that copy because the copy would appear to be useless without considering what coro_suspend does, which is why we are changing BasicAA to prevent such elimination.

If an argument is just a pointer without any argument attributes, BasicAA will assume coro_suspend frees the underlying allocations. Not because it has any particular knowledge of how coro_suspend works, but because that's the default assumption for any function call with unknown semantics.

BasicAA treats arguments marked byval differently because it understands the semantics of byval: it uses escape analysis to prove an arbitrary function call can't modify the contents.

---

I guess in some sense, it's possible to specify a world where coro_suspend frees byval allocations. But such a world doesn't really make sense. And we'd need to audit every pass that knows anything about byval semantics, so I really don't want to go down that path.

only check byval attribute

From clang's perspective, what it needs to ensure is that every argument is stored in an allocation local to the callee (in other words, allocations corosplit can handle). "allocations corosplit can handle" should be either allocas, or arguments marked byval. These are pointers that don't exist in the caller, so corosplit can mess with the address in memory without worrying about code it can't see in the caller.

How do you prevent optimization passes (prior to CoroSplit) from messing up with those pointers, though?
Let's say we have an argument that's not marked with byval (due to target specifics) but is pass-by-value in C++. In Clang we could emit IR to copy the argument value to a local alloca. However latter optimization passes could very well optimize out that copy because the copy would appear to be useless without considering what coro_suspend does, which is why we are changing BasicAA to prevent such elimination.
Or are you suggesting we need to modify Clang such that every pass-by-value argument in C++ must be marked with byval?

In D104007#2815847, @efriedma wrote:

In D104007#2815732, @lxfind wrote:

It seems that there is no stable contract to tell whether an argument is byval, and it depends on the target.
For example: https://godbolt.org/z/YcrYPbMdc
So we won't be able to fix this just in CoroSplit pass. Either we change the front-end to force all byval arguments to have byval attribute (not sure if that's possible), or we may have to conservatively go with this patch.

I'm not sure what you mean. An LLVM IR argument either has the byval attribute, or it doesn't. If it does, the allocation belongs to the callee. If it doesn't, the pointer refers to an allocation constructed in the caller. There isn't any other sense in which an argument can be "byval".

A given C++ function can have a bunch of variations in the LLVM IR signature, depending on the target, but that's not relevant here. Once clang is finished emitting IR, the C++ type signature becomes irrelevant.

Unfortunately what we really care about here is whether an argument is by-value from C++ perspective, not just whether it has a byval attribute. Let me try to explain the problem top-down and see if I got it wrong somewhere:

1. In C++, when we call a function with a parameter passed by value, such as foo(std::move(a)), the passed value is typically a temporary value allocated at caller, and only the pointer is passed to the callee. When callee returns, the caller will be able to deallocate that temporary value's memory (in LLVM IR terms, there will be a lifetime.end marking the temporary value after the callee returns).
2. If the callee is a normal routine (non-coroutine), the callee don't need to worry about the lifetime of the parameter because it will stay alive through the entire life cycle of the callee.
3. However if the callee is a coroutine, the situation is different because the coroutine can return in the middle of the function and latter resume. As soon as the coroutine returns for the first time (i.e. suspended the first time), the temporary value (the pass-by-value parameter) would die in the caller. But if the callee needs to use that argument after coroutine resumption, we need to make sure that the argument value will be put on the coroutine frame (similar to allocas), so that latter access won't be accessing invalid memory.
4. However there is one major difference between a parameter and an alloca: for a parameter that is a pointer type, we need to know whether we need to copy the value that the pointer points to to the coroutine frame, or just the pointer it self. For instance, you could call a coroutine by passing a direct pointer, and in that case we should only need to copy the pointer to the frame. But for the C++ pass-by-value arguments, we need to copy the value that the pointer points to to the frame. The question is then, during CoroSplit (the pass where we put things to the frame), are we able to tell, given an argument pointer, whether we need to copy the value or just the pointer to the frame? Knowing that a C++ pass-by-value argument doesn't necessarily end up with an argument with a byval attribute, we know that it is not possible to tell, hence we cannot deal with this properly only in CoroSplit pass.

That this sound right?

In D104007#2814244, @efriedma wrote:

In D104007#2813714, @efriedma wrote:

Do you know if there is a stable contract in IR to tell whether an argument is in fact a callee-owned memory?

Argument::hasPassPointeeByValueCopyAttr()

Sorry, that's not right. The question you want to answer here is whether the pointer in the caller points to different memory from the pointer in the callee. The only attribute that causes that is byval, and it's very unlikely we'll ever add any others.

inalloca/preallocated memory is part of the callee stack frame, at a low level. But it's actually allocated in the caller, so from an alias-analysis perspective, it doesn't count as a local allocation like byval would.

I just realized that this is now getting very similar to D101980, and we are facing the same problem of not always be able to tell whether a param is byval in the midend. Let me continue the discussion on the BasicAA patch.

In D104007#2814244, @efriedma wrote:

In D104007#2813714, @efriedma wrote:

Do you know if there is a stable contract in IR to tell whether an argument is in fact a callee-owned memory?

Argument::hasPassPointeeByValueCopyAttr()

Sorry, that's not right. The question you want to answer here is whether the pointer in the caller points to different memory from the pointer in the callee. The only attribute that causes that is byval, and it's very unlikely we'll ever add any others.

inalloca/preallocated memory is part of the callee stack frame, at a low level. But it's actually allocated in the caller, so from an alias-analysis perspective, it doesn't count as a local allocation like byval would.

hmm sounds like hasPassPointeeByValueCopyAttr() is actually what I need. In CoroSplit, when deciding what to put on the coroutine frame, for arguments, I will need to decide whether it's enough to just put the pointer on the frame, or if I need to copy the content of the pointer (much like allocas) to the frame. Sounds like for inalloca and preallocated memory I still want to copy the content to the frame, not just the pointer.

Poking around it looks like this should be handled for inlining, not sure why the previous case we ran into wasn't covered by that code below from CallAnalyzer::analyze()

// Disallow inlining a blockaddress with uses other than strictly callbr.
// A blockaddress only has defined behavior for an indirect branch in the
// same function, and we do not currently support inlining indirect
// branches.  But, the inliner may not see an indirect branch that ends up
// being dead code at a particular call site. If the blockaddress escapes
// the function, e.g., via a global variable, inlining may lead to an
// invalid cross-function reference.
// FIXME: pr/39560: continue relaxing this overt restriction.
if (BB->hasAddressTaken())
  for (User *U : BlockAddress::get(&*BB)->users())
    if (!isa<CallBrInst>(*U))
      return InlineResult::failure("blockaddress used outside of callbr");

LGTM, thanks for the fix.

In D104007#2812536, @efriedma wrote:

If you fix the clang code so it isn't generating these extra useless allocas, the problem with byval should become obvious: even without any optimizations, we end up with a use-after-free. corosplit isn't preserving memory that's supposed to be part of the callee frame.

In D104007#2812401, @efriedma wrote:

In D104007#2812395, @lxfind wrote:

In D104007#2812383, @efriedma wrote:

This doesn't seem right. A byval argument is essentially a local variable. We should treat it the same way as we would an alloca. If it's live across the first suspend point, it should become part of the coroutine frame.

It will become part of the coroutine frame, by copying it through memcpy. That's actually what's the first memcpy for in the test case.

I don't understand. As far as I can tell, the destination of both memcpys in the testcase are allocas.

OK, so the way coroutine codegen works is to first copy all parameters to local alloca, and then if any of those allocas need to live through coroutine suspension, the alloca will be put on the coroutine frame during the CoroSplit pass.

In D104007#2812383, @efriedma wrote:

This doesn't seem right. A byval argument is essentially a local variable. We should treat it the same way as we would an alloca. If it's live across the first suspend point, it should become part of the coroutine frame.

In D104007#2812383, @efriedma wrote:

This doesn't seem right. A byval argument is essentially a local variable. We should treat it the same way as we would an alloca. If it's live across the first suspend point, it should become part of the coroutine frame.

address comment

I found a more proper fix in D104007.

more test

Only skip regions that contain BBs with address taken

Add test case

In D103867#2806612, @aeubanks wrote:

do all passes that clone BBs have to worry about this?

I think so, as long as the cloned BB is in the same module as the original one, we need to worry about this (there are legit cloning in some tools such as llvm-extract, llvm-reduce..)