Great! To my understanding, if there are lifetime informations, the behavior of this patch is same with the behavior of previous implementation. Is my understanding right?

In D88872#2323195, @lxfind wrote:

In D88872#2323145, @ChuanqiXu wrote:

In D88872#2321902, @lxfind wrote:

In D88872#2320823, @ChuanqiXu wrote:

Thanks for your patch! It mentions some bugs about allocas we need to handle in the future.

For this patch, I'm little confusing about why we need to separate alloca from spills. In my mind, a spill means something we need to put in the frame. And an alloca which would be in the frame is naturally a spill.
I think the patch benefits from replacing

using SpillInfo = SmallVector<Spill, 8>;

to

using SpillInfo = SmallMapVector<Value *, SmallVector<Instruction *, 2>, 8>;

Thanks for taking a look at the patch. If you look at the implementation, the handling of "Spills" and always different from the handling of allocas. They do share the same concept that they need to go to the frame (which is why they both belong to FrameDataInfo).
The primary reason to separate them (and hence set up the code for future fixes), is this one primary difference between them: A Spill is a direct def-use relationship that crosses suspension points; while an alloca may not be exposed to a direct def-use relationship that crosses suspension points but still need to go to the frame. The condition for them to go to the frame is fundamentally different.

I agree with we can benefit from separating alloca from spills. At least we don't need extract allocas from spills by a redundant loop any more. After we separate allocas from spills, the name spills seems a little strange. But I think it doesn't really matter.

Here is a question about the bug mentioned in summary. I write a simple code like this:

void consuming(int* pa);

task escape_alloca() {
  int a;
  consuming(&a);
  co_await something();
}

But clang would still put a in the frame in O0 or O2. I guess it is because the def of a and the use of a is cross initial_suspend (in O0 mode) or the lifetime markers is cross the co_await something point. Is there anything wrong? Or what is the example about the bug?

It's harder to generate an example from source code (it has to be quite complicated, I have some production code, but not small enough to share). But it's easy to see from IR examples.
Consider the following IR (you can run it through opt --coro-split:

define i8* @f(i1 %n) "coroutine.presplit"="1" {
entry:
  %a = alloca i32, align 8
  %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
  %size = call i32 @llvm.coro.size.i32()
  %alloc = call i8* @malloc(i32 %size)
  %hdl = call i8* @llvm.coro.begin(token %id, i8* %alloc)

  %flag = call i1 @check()
  br i1 %flag, label %flag_true, label %flag_false

flag_true:
  %b = bitcast i32* %a to i8*
  br label %merge

flag_false:
  %c = bitcast i32* %a to i8*
  br label %merge

merge:
  %d = phi i8* [ %b, %flag_true ], [ %c, %flag_false ]
  %sp1 = call i8 @llvm.coro.suspend(token none, i1 false)
  switch i8 %sp1, label %suspend [i8 0, label %resume
                                  i8 1, label %cleanup]
resume:
  call void @print(i8* %d)
  br label %cleanup

cleanup:
  %mem = call i8* @llvm.coro.free(token %id, i8* %hdl)
  call void @free(i8* %mem)
  br label %suspend
suspend:
  call i1 @llvm.coro.end(i8* %hdl, i1 0)
  ret i8* %hdl
}

declare i8* @llvm.coro.free(token, i8*)
declare i32 @llvm.coro.size.i32()
declare i8  @llvm.coro.suspend(token, i1)
declare void @llvm.coro.resume(i8*)
declare void @llvm.coro.destroy(i8*)

declare token @llvm.coro.id(i32, i8*, i8*, i8*)
declare i1 @llvm.coro.alloc(token)
declare i8* @llvm.coro.begin(token, i8*)
declare i1 @llvm.coro.end(i8*, i1)

declare noalias i8* @malloc(i32)
declare i1 @check()
declare void @print(i8*)
declare void @free(i8*)

Notice that %a's alias is stored in a PHI, which is used after suspend. When you run coro-split on it, the current implementation will think that only the PHI needs to be spilled, while %a can stay on stack.
So in the generated IR, %a will still be an alloca, but a pointer to it will be store to the frame! The generated IR looks like this:

define i8* @f(i1 %n) {
entry:
  %a = alloca i32, align 8
  ...
  %b = bitcast i32* %a to i8*
  %d.spill.addr = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 2
  store i8* %b, i8** %d.spill.addr, align 8
  ...
}

This is just one example, but you can also escape it by storing the address, or call a function.

In D88872#2321902, @lxfind wrote:

In D88872#2320823, @ChuanqiXu wrote:

Thanks for your patch! It mentions some bugs about allocas we need to handle in the future.

For this patch, I'm little confusing about why we need to separate alloca from spills. In my mind, a spill means something we need to put in the frame. And an alloca which would be in the frame is naturally a spill.
I think the patch benefits from replacing

using SpillInfo = SmallVector<Spill, 8>;

to

using SpillInfo = SmallMapVector<Value *, SmallVector<Instruction *, 2>, 8>;

Thanks for taking a look at the patch. If you look at the implementation, the handling of "Spills" and always different from the handling of allocas. They do share the same concept that they need to go to the frame (which is why they both belong to FrameDataInfo).
The primary reason to separate them (and hence set up the code for future fixes), is this one primary difference between them: A Spill is a direct def-use relationship that crosses suspension points; while an alloca may not be exposed to a direct def-use relationship that crosses suspension points but still need to go to the frame. The condition for them to go to the frame is fundamentally different.

Thanks for your patch! It mentions some bugs about allocas we need to handle in the future.

rename wouldOptimize to ReuseFrameSlot.

instead OptLevel in CoroSplit with bool to control whether the compiler would optimize a coroutine.

Remove unnecessary headers and declarations.

In D87596#2287989, @lxfind wrote:

Overall LGTM. Thank you for addressing the feedback!
One last thing, the optimization level code can be further simplified. Since you are obtaining the OptLevel from the PassManagerBuilder, which is an integer. I feel it would be easier if you just stick with an integer optlevel instead of converting it to OptimizationLevel. The reason OptimizationLevel exists is to be able to model both OptLevel and SizeLevel at the same time, but here you don't really care about the SizeLevel. So I would suggest just use an int for it and save all the trouble converting.

Change the type of OptLevel in Coro Split Passes as unsigned instead of PassBuilder::OptimizationLevel.

• Remove the feature that we can pass optimization level to opt tool like -passes='coro-split<On>'
• remove redundant addFieldForAlloca and addField

• Move all the implementation details into addFieldForAllocas function.
• Change the type of ForSpill member of Field from pointer to a SmallVector of Spill.

Gate this optimization under optimization level and put the optimization code into a helper class.

In D87596#2272955, @lxfind wrote:

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.

That's not entirely true though. lifetime markers could be enabled with ASAN in O0 for instance.

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?

In D86672#2240988, @vitalybuka wrote:

isPointerOffset is not what I need

In D85399#2225025, @vitalybuka wrote:

LGTM with some updates

In D85399#2223329, @vitalybuka wrote:

The test is still could be useful
Maybe something like this llvm/test/Analysis/StackSafetyAnalysis/lifetime.ll ?

In D85399#2223309, @vitalybuka wrote:

This function was changed
Is this issue still relevant?
If this is still needed, could you please make this a regular lit test?

@RKSimon , gentle ping~

In D85399#2199566, @RKSimon wrote:

test case?

Update test case

In D85279#2195758, @junparser wrote:

LGTM，Thank you, Please merge the testcases into one file.

Create BitCastInst directly instead of using IRBuilder

Due to my mistake, the diff updated before is a draft. It is terribly sorry if it confuses any one...

In D84821#2183795, @ChuanqiXu wrote:

In D84821#2183027, @nikic wrote:

I don't think this is the right approach to the problem. Can you give some more information on which places in LLVM assume that only bitcasts feed into lifetime intrinsics?

From the CodeExtractor.cpp, CoroFrame.cpp and StackLifetime.cpp, I find such assumptions. I also find that there are other methods to collect lifetime marker of an instruction. For example, in the Inline Module, it finds among all the users of the instruction I to find which is used by lifetime marker. And in the InstCombineLoadStore.cpp, I find it collects lifetime marker by calculates the users with kind BitCast, GEP and AddrSpaceCastInst (which means there are cases which are not covered by this patch). What I want to say here is that the style of collecting lifetime marker seems a little out-of-order.

In D84821#2183271, @lebedev.ri wrote:

I'm not sure any such assumption being made is sane. Those passes should be fixed instead.

Yes. We can fix this problem by either fixing other passes or fixing this pass. We need find out that which one is more natural. In my thought, when I want to collect lifetime marker as a newbie here, I will go to see how the lifetime marker is created. So I find CreateLifetimeStart and CreateLifetimeEnd in LLVM and CreateCall ('lifetime_start' or lifetime_end`') in clang's CodeGen` module. In all of the methods, the logic is to judge whether the type of the source value is i8*. If the type of the source value is i8*, the Create* methods will create a direct call to lifetime intrinsics. Otherwise, the Create* methods will create a BitCast to convert the type of source value to i8*. So from the process, I think it is natural for code reader who is not so familiar with LLVM to think that he can collect lifetime marker by judge the instruction itself and the users who is BitCastInst.

In D84821#2183027, @nikic wrote:

I don't think this is the right approach to the problem. Can you give some more information on which places in LLVM assume that only bitcasts feed into lifetime intrinsics?

In D84135#2162478, @MaskRay wrote:

For NFC changes please tag so (you can find plenty of examples in the history). Please upload the full diff (arc diff or git diff 'HEAD^' -U9999).

---

When you commit, please 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.

Hi @modocache ，may I ask for your option about this patch?

re-upload the patch due to previous misuse of Phabricator , sorry..

Because of D82029, the promise_type::final_suspend() should declare with noexcept specifier .