Page MenuHomePhabricator

ChuanqiXu (Chuanqi Xu)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 22 2020, 11:08 PM (31 w, 2 d)

Recent Activity

Tue, Jan 26

ChuanqiXu added a comment to D94834: [Coroutine] Do not CoroElide if there are musttail calls.

And I think there are two cases about this example:
(1) The musttail call is dominated by @llvm.subfn.addr(%handle, 1) calls. Then %handle is destroyed at the musttail call, so it makes no sense that the musttail call returns %handle.
(2) The musttail call isn't dominated by @llvm.subfn.addr(%handle, 1) calls. Then the musttail call may returns %handle. And it doesn't matter since %handle won't be elided in this case.

In case (1), why is it that '%handle' must be destroyed at the musttail call?

In my mind, @llvm.subfn.addr(%handle, 1) means a call which would destroy %handle. And CoroElide Pass also treat the call as a lifetime end marker for %handle. CoroElide pass only elides coroutine handle who is guarded by @llvm.subfn.addr(%handle, 1) .

I guess that means we could improve this by teaching alias analysis that llvm.subfn.addr kills the handle.

Tue, Jan 26, 5:51 PM · Restricted Project

Mon, Jan 25

ChuanqiXu added a comment to D94834: [Coroutine] Do not CoroElide if there are musttail calls.

And I think there are two cases about this example:
(1) The musttail call is dominated by @llvm.subfn.addr(%handle, 1) calls. Then %handle is destroyed at the musttail call, so it makes no sense that the musttail call returns %handle.
(2) The musttail call isn't dominated by @llvm.subfn.addr(%handle, 1) calls. Then the musttail call may returns %handle. And it doesn't matter since %handle won't be elided in this case.

In case (1), why is it that '%handle' must be destroyed at the musttail call?

Mon, Jan 25, 6:07 PM · Restricted Project
ChuanqiXu added a comment to D94834: [Coroutine] Do not CoroElide if there are musttail calls.

I am a little unclear about this problem. From my point of view, it seems like that there is a Coroutine C elided in a normal function F. Then in the Coroutine Body of C, it would try to switch to itself by symmetric transfer. However, the Coroutine frame of C now is a stack variable. Then the tail call would pass the address of the stack variable whose lifetime has ended, so here is the corruption. Did I understand the situation?

Yes I believe that's an accurate description

I'm looking into this bug now. Now it is more confusing to me since it seems like that this process shouldn't happen.

The IR generated for the attachment of bug48626 is like:

%19 = tail call noalias nonnull i8* @llvm.coro.begin(token %15, i8* %18), !noalias !1082 ; The handle we are going to elide
// ... in another BB
%35 = call i8* @llvm.coro.subfn.addr(i8* nonnull %19, i8 1) #28 ; destroy %19
// ... in another BB
musttail call fastcc void %51(i8* %retval.sroa.0.0.copyload.i.i.i120) ; dominated by `call i8* @llvm.coro.subfn.addr(i8* nonnull %19, i8 1)`
ret void

It seems like that the argument of the musttail call can't alias %19 in any sense since %19 already been destroyed before the musttail call. Then it makes sense to all situations I can image. Since we only elide the coro handles who wouldn't escape. And we did the escape analysis very conservatively (Not by AA analysis, but by a simple path based escape analysis). So I think it may be better to remove the check in the end of ShouldElide function. It seems like that the memory wouldn't crash due to the argument of the musttail call alias with the coroutine handle we elided. And if any corruption happens, I think is is a bug for the escape analysis.

It won't crash in this particular case. But as discussed in the bug thread, in theory the symmetric transfer could return the same handle as passed in, and in that case you won't be able to do tail call, right?

Mon, Jan 25, 5:56 PM · Restricted Project
ChuanqiXu added a comment to D94834: [Coroutine] Do not CoroElide if there are musttail calls.

I am a little unclear about this problem. From my point of view, it seems like that there is a Coroutine C elided in a normal function F. Then in the Coroutine Body of C, it would try to switch to itself by symmetric transfer. However, the Coroutine frame of C now is a stack variable. Then the tail call would pass the address of the stack variable whose lifetime has ended, so here is the corruption. Did I understand the situation?

Yes I believe that's an accurate description

Mon, Jan 25, 3:42 AM · Restricted Project

Wed, Jan 20

ChuanqiXu committed rGc1bc7981babc: [Coroutine] Remain alignment information when merging frame variables (authored by ChuanqiXu).
[Coroutine] Remain alignment information when merging frame variables
Wed, Jan 20, 2:59 AM
ChuanqiXu closed D94891: [Coroutine] Remain alignment information when merging frame variables.
Wed, Jan 20, 2:59 AM · Restricted Project

Tue, Jan 19

ChuanqiXu updated the diff for D94891: [Coroutine] Remain alignment information when merging frame variables.

Refine typos.

Tue, Jan 19, 11:34 PM · Restricted Project
ChuanqiXu updated the diff for D94891: [Coroutine] Remain alignment information when merging frame variables.

Remove unintentional change.

Tue, Jan 19, 5:58 PM · Restricted Project
ChuanqiXu added inline comments to D94891: [Coroutine] Remain alignment information when merging frame variables.
Tue, Jan 19, 5:48 PM · Restricted Project
ChuanqiXu added a comment to D94891: [Coroutine] Remain alignment information when merging frame variables.

Thanks for explaining! I visited the his profile. It is really confusing.

I also noticed the same when the clang-format project got removed (archived), along with a whole load of other projects. From what I can tell this person seems to be making changes which from what I can tell I don't understand why.

Tue, Jan 19, 1:47 AM · Restricted Project
ChuanqiXu updated subscribers of D94891: [Coroutine] Remain alignment information when merging frame variables.
Tue, Jan 19, 1:34 AM · Restricted Project
ChuanqiXu updated subscribers of D94891: [Coroutine] Remain alignment information when merging frame variables.

@231084fg may I ask why you remove the reviews for the patch?

This person is user since only 15th Jan, I'm beginning to think they may be malicious!

@hans , @rsmith , @benhamilton anyone mind if I disable this users account? they seem to be randomly going through phabricator making changes which seem to be unrelated.

Their email is also not verified.

Tue, Jan 19, 1:26 AM · Restricted Project

Mon, Jan 18

ChuanqiXu added a comment to D94891: [Coroutine] Remain alignment information when merging frame variables.

@231084fg may I ask why you remove the reviews for the patch?

Mon, Jan 18, 7:01 PM · Restricted Project
ChuanqiXu added reviewers for D94891: [Coroutine] Remain alignment information when merging frame variables: jmorse, junparser, lxfind.
Mon, Jan 18, 6:59 PM · Restricted Project
ChuanqiXu added a comment to D94891: [Coroutine] Remain alignment information when merging frame variables.

@jmorse gentle ping~

Mon, Jan 18, 6:06 PM · Restricted Project

Sun, Jan 17

ChuanqiXu added a comment to D94834: [Coroutine] Do not CoroElide if there are musttail calls.

I am a little unclear about this problem. From my point of view, it seems like that there is a Coroutine C elided in a normal function F. Then in the Coroutine Body of C, it would try to switch to itself by symmetric transfer. However, the Coroutine frame of C now is a stack variable. Then the tail call would pass the address of the stack variable whose lifetime has ended, so here is the corruption. Did I understand the situation?

Sun, Jan 17, 10:22 PM · Restricted Project
ChuanqiXu requested review of D94891: [Coroutine] Remain alignment information when merging frame variables.
Sun, Jan 17, 7:23 PM · Restricted Project

Dec 15 2020

ChuanqiXu committed rG8b48d2437320: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid… (authored by ChuanqiXu).
[clang-format] Recognize c++ coroutine keywords as unary operator to avoid…
Dec 15 2020, 4:52 AM
ChuanqiXu closed D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.
Dec 15 2020, 4:52 AM · Restricted Project, Restricted Project
ChuanqiXu added a comment to D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.

I don't like seeing users having to use `// clang-format off```

auto try_sequence = [](int& ref) -> return_ignore {
            /* clang-format off */
            for co_await(int v : return_random_int())
                ref = v;
            /* clang-format on*/
        };

it would be nice if we could land a co_routines improvement

Dec 15 2020, 4:46 AM · Restricted Project, Restricted Project
ChuanqiXu added a comment to D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.

Aside: Would you be prepared to take a look at D34225: [clang-format] Teach clang-format how to handle C++ coroutines which is pretty much been abandoned, and see if there is anything there that might help the co_routinues support?

Dec 15 2020, 4:15 AM · Restricted Project, Restricted Project
ChuanqiXu added inline comments to D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.
Dec 15 2020, 2:46 AM · Restricted Project, Restricted Project
ChuanqiXu updated the diff for D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.

Edit the comment to avoid misleading.

Dec 15 2020, 2:45 AM · Restricted Project, Restricted Project

Dec 14 2020

ChuanqiXu added inline comments to D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.
Dec 14 2020, 11:42 PM · Restricted Project, Restricted Project
ChuanqiXu updated the diff for D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.

add verifyFormat("return *a", PASLeftStyle); to clarify the change

Dec 14 2020, 9:53 PM · Restricted Project, Restricted Project

Dec 13 2020

ChuanqiXu added a comment to D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.

Peanut gallery says: It seems to me that your root problem here is that co_yield is not being recognized by the parser as a keyword, and so you're seeing e.g. co_yield* p; the same way you'd see CoYield* p;.
But in that case, you should also be seeing co_yield -1; formatted as co_yield - 1; the same way you'd see CoYield - 1;.
Is it possible that you're not using the right (C++20) language mode? (OTOH, see D69144, D69180, which also didn't concern themselves with language modes. I don't understand why not.)

Could you please re-upload the diff with context (i.e. git show -U999)?

Dec 13 2020, 9:52 PM · Restricted Project, Restricted Project
ChuanqiXu updated the diff for D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.

Re-upload patch with context

Dec 13 2020, 9:44 PM · Restricted Project, Restricted Project
ChuanqiXu added a comment to D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.

What does the behavior goes wrong mean? why can't it be left aligned?

Dec 13 2020, 9:41 PM · Restricted Project, Restricted Project

Nov 25 2020

ChuanqiXu added a comment to D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.

@djasper gental ping~

Nov 25 2020, 5:44 PM · Restricted Project, Restricted Project

Nov 19 2020

Herald added a project to D35816: [Greedy RegAlloc] Add logic to greedy reg alloc to avoid bad eviction chains: Restricted Project.
Nov 19 2020, 5:59 PM · Restricted Project

Nov 11 2020

ChuanqiXu committed rGcd89c4dbdd3a: [NFC][coroutines] remove unused argument in SemaCoroutine (authored by ChuanqiXu).
[NFC][coroutines] remove unused argument in SemaCoroutine
Nov 11 2020, 9:24 PM
ChuanqiXu closed D91243: [NFC][Coroutines] Remove unused `IsImplicit` argument in maybeTailCall.
Nov 11 2020, 9:24 PM · Restricted Project
ChuanqiXu updated the diff for D91243: [NFC][Coroutines] Remove unused `IsImplicit` argument in maybeTailCall.

Remove unused IsImplicit argument in buildCoawaitCalls.

Nov 11 2020, 5:58 PM · Restricted Project
ChuanqiXu added a comment to D91305: [Coroutine] Allocas used by StoreInst does not always escape.

Good. The intuition and implementation make sense.

Nov 11 2020, 5:55 PM · Restricted Project
ChuanqiXu requested review of D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment.
Nov 11 2020, 3:44 AM · Restricted Project, Restricted Project
ChuanqiXu requested review of D91243: [NFC][Coroutines] Remove unused `IsImplicit` argument in maybeTailCall.
Nov 11 2020, 3:04 AM · Restricted Project

Oct 22 2020

ChuanqiXu added a comment to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.

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?

Oct 22 2020, 7:11 PM · Restricted Project

Oct 10 2020

ChuanqiXu added a comment to D88872: [Coroutines] Refactor/Rewrite Spill and Alloca processing.

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.

Oct 10 2020, 1:23 AM · Restricted Project

Oct 9 2020

ChuanqiXu added a comment to D88872: [Coroutines] Refactor/Rewrite Spill and Alloca processing.

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.

Oct 9 2020, 8:13 PM · Restricted Project

Oct 8 2020

ChuanqiXu added a comment to D88872: [Coroutines] Refactor/Rewrite Spill and Alloca processing.

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

Oct 8 2020, 11:28 PM · Restricted Project

Sep 28 2020

ChuanqiXu committed rGb3a722e66b75: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes (authored by ChuanqiXu).
[Coroutines] Reuse storage for local variables with non-overlapping lifetimes
Sep 28 2020, 12:48 AM
ChuanqiXu closed D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 28 2020, 12:48 AM · Restricted Project

Sep 27 2020

ChuanqiXu updated the diff for D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.

rename wouldOptimize to ReuseFrameSlot.

Sep 27 2020, 2:54 AM · Restricted Project

Sep 25 2020

ChuanqiXu added inline comments to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 25 2020, 2:16 AM · Restricted Project
ChuanqiXu updated the diff for D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.

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

Sep 25 2020, 2:14 AM · Restricted Project

Sep 24 2020

ChuanqiXu added inline comments to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 24 2020, 2:39 AM · Restricted Project
ChuanqiXu updated the diff for D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.

Remove unnecessary headers and declarations.

Sep 24 2020, 2:38 AM · Restricted Project

Sep 22 2020

ChuanqiXu added a comment to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.

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.

Sep 22 2020, 11:45 PM · Restricted Project
ChuanqiXu updated the diff for D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.

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

Sep 22 2020, 11:45 PM · Restricted Project
ChuanqiXu added inline comments to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 22 2020, 1:20 AM · Restricted Project
ChuanqiXu updated the diff for D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
  • Remove the feature that we can pass optimization level to opt tool like -passes='coro-split<On>'
  • remove redundant addFieldForAlloca and addField
Sep 22 2020, 1:17 AM · Restricted Project

Sep 21 2020

ChuanqiXu added inline comments to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 21 2020, 11:07 PM · Restricted Project
ChuanqiXu updated the diff for D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
  • Move all the implementation details into addFieldForAllocas function.
  • Change the type of ForSpill member of Field from pointer to a SmallVector of Spill.
Sep 21 2020, 10:55 PM · Restricted Project

Sep 20 2020

ChuanqiXu added inline comments to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 20 2020, 2:17 AM · Restricted Project

Sep 17 2020

ChuanqiXu updated the diff for D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.

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

Sep 17 2020, 12:12 AM · Restricted Project

Sep 14 2020

ChuanqiXu added inline comments to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 14 2020, 9:47 PM · Restricted Project
ChuanqiXu added inline comments to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 14 2020, 8:55 PM · Restricted Project
ChuanqiXu added a comment to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.

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.

Sep 14 2020, 8:10 PM · Restricted Project
ChuanqiXu added inline comments to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 14 2020, 8:06 PM · Restricted Project
ChuanqiXu added a comment to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.

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?

Sep 14 2020, 8:02 PM · Restricted Project

Sep 13 2020

ChuanqiXu requested review of D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 13 2020, 11:09 PM · Restricted Project

Sep 3 2020

ChuanqiXu added inline comments to D86859: [Coroutine] Make dealing with alloca spills more robust.
Sep 3 2020, 6:30 PM · Restricted Project
ChuanqiXu added inline comments to D86859: [Coroutine] Make dealing with alloca spills more robust.
Sep 3 2020, 6:28 PM · Restricted Project
ChuanqiXu added inline comments to D86859: [Coroutine] Make dealing with alloca spills more robust.
Sep 3 2020, 1:29 AM · Restricted Project
ChuanqiXu added inline comments to D86859: [Coroutine] Make dealing with alloca spills more robust.
Sep 3 2020, 1:14 AM · Restricted Project

Aug 26 2020

ChuanqiXu added a comment to D86672: [StackSafety] Ignore allocas with partial lifetime markers.

isPointerOffset is not what I need

Aug 26 2020, 10:43 PM · Restricted Project
ChuanqiXu added inline comments to D84630: [StackSafety] Skip ambiguous lifetime analysis.
Aug 26 2020, 6:34 PM · Restricted Project
ChuanqiXu added inline comments to D84630: [StackSafety] Skip ambiguous lifetime analysis.
Aug 26 2020, 12:12 AM · Restricted Project

Aug 18 2020

ChuanqiXu added a comment to D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.

LGTM with some updates

Aug 18 2020, 6:33 PM · Restricted Project
ChuanqiXu added a comment to D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.

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

Aug 18 2020, 4:04 AM · Restricted Project
ChuanqiXu updated the diff for D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.
Aug 18 2020, 4:02 AM · Restricted Project
ChuanqiXu added a comment to D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.

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

Aug 18 2020, 2:37 AM · Restricted Project

Aug 12 2020

ChuanqiXu added a comment to D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.

@RKSimon , gentle ping~

Aug 12 2020, 11:17 PM · Restricted Project

Aug 6 2020

ChuanqiXu added a comment to D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.

test case?

Aug 6 2020, 7:42 PM · Restricted Project
ChuanqiXu updated the diff for D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.

Update test case

Aug 6 2020, 7:41 PM · Restricted Project
ChuanqiXu updated the diff for D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.
Aug 6 2020, 7:38 PM · Restricted Project

Aug 5 2020

ChuanqiXu closed D85279: [Coroutines] Use `Value::stripPointerCasts` to collect lifetime marker of `alloca` in CoroFrame.
Aug 5 2020, 11:56 PM · Restricted Project
ChuanqiXu updated the summary of D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.
Aug 5 2020, 11:56 PM · Restricted Project
ChuanqiXu requested review of D85399: [NFC][StackSafety] Test that StackLifetime looks through stripPointerCasts.
Aug 5 2020, 11:45 PM · Restricted Project
ChuanqiXu committed rG92f1f1e40d4c: [Coroutines] Use to collect lifetime marker of in CoroFrame Differential… (authored by ChuanqiXu).
[Coroutines] Use to collect lifetime marker of in CoroFrame Differential…
Aug 5 2020, 11:22 PM
ChuanqiXu updated the summary of D85279: [Coroutines] Use `Value::stripPointerCasts` to collect lifetime marker of `alloca` in CoroFrame.
Aug 5 2020, 7:24 PM · Restricted Project
ChuanqiXu added a comment to D85279: [Coroutines] Use `Value::stripPointerCasts` to collect lifetime marker of `alloca` in CoroFrame.

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

Aug 5 2020, 7:23 PM · Restricted Project
ChuanqiXu updated the diff for D85279: [Coroutines] Use `Value::stripPointerCasts` to collect lifetime marker of `alloca` in CoroFrame.

Create BitCastInst directly instead of using IRBuilder

Aug 5 2020, 7:09 PM · Restricted Project
ChuanqiXu updated the diff for D85279: [Coroutines] Use `Value::stripPointerCasts` to collect lifetime marker of `alloca` in CoroFrame.

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

Aug 5 2020, 12:34 AM · Restricted Project
ChuanqiXu abandoned D85279: [Coroutines] Use `Value::stripPointerCasts` to collect lifetime marker of `alloca` in CoroFrame.
Aug 5 2020, 12:22 AM · Restricted Project
ChuanqiXu requested review of D85279: [Coroutines] Use `Value::stripPointerCasts` to collect lifetime marker of `alloca` in CoroFrame.
Aug 5 2020, 12:16 AM · Restricted Project

Aug 3 2020

ChuanqiXu abandoned D84821: [InstCombine] Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker.
Aug 3 2020, 1:50 AM · Restricted Project

Aug 2 2020

ChuanqiXu closed D84135: [NFC][IR] add comments and a unit test for User::replaceUsesOfWith.
Aug 2 2020, 8:19 PM · Restricted Project

Jul 30 2020

ChuanqiXu added a comment to D84821: [InstCombine] Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker.

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.

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.

Jul 30 2020, 3:57 AM · Restricted Project

Jul 29 2020

ChuanqiXu added a comment to D84821: [InstCombine] Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker.

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?

Jul 29 2020, 10:52 PM · Restricted Project

Jul 28 2020

ChuanqiXu retitled D84821: [InstCombine] Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker from Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker to [InstCombine] Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker.
Jul 28 2020, 8:46 PM · Restricted Project
ChuanqiXu requested review of D84821: [InstCombine] Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker.
Jul 28 2020, 8:46 PM · Restricted Project
ChuanqiXu committed rGd3527052fc2a: [NFC] Edit the comment for the return type of await_suspend (authored by ChuanqiXu).
[NFC] Edit the comment for the return type of await_suspend
Jul 28 2020, 7:22 PM
ChuanqiXu committed rGdd4106d22ef6: [NFC] Edit the comment in User::replaceUsesOfWith (authored by ChuanqiXu).
[NFC] Edit the comment in User::replaceUsesOfWith
Jul 28 2020, 7:02 PM

Jul 22 2020

ChuanqiXu updated the diff for D84137: [NFC][Coroutines] Update the comment in SemaCoroutine.cpp for the return type of await_suspend().
Jul 22 2020, 8:26 PM
ChuanqiXu added a comment to D84135: [NFC][IR] add comments and a unit test for User::replaceUsesOfWith.

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.

Jul 22 2020, 8:25 PM · Restricted Project
ChuanqiXu updated the diff for D84135: [NFC][IR] add comments and a unit test for User::replaceUsesOfWith.
Jul 22 2020, 8:22 PM · Restricted Project
ChuanqiXu retitled D84135: [NFC][IR] add comments and a unit test for User::replaceUsesOfWith from [IR] add comments and a unit test for User::replaceUsesOfWith to [NFC][IR] add comments and a unit test for User::replaceUsesOfWith.
Jul 22 2020, 8:16 PM · Restricted Project

Jul 20 2020

ChuanqiXu added a reviewer for D84137: [NFC][Coroutines] Update the comment in SemaCoroutine.cpp for the return type of await_suspend(): junparser.
Jul 20 2020, 12:10 AM
ChuanqiXu created D84137: [NFC][Coroutines] Update the comment in SemaCoroutine.cpp for the return type of await_suspend().
Jul 20 2020, 12:10 AM