Page MenuHomePhabricator

junparser (JunMa)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 11 2019, 11:18 PM (98 w, 28 m)

Recent Activity

Fri, Jan 22

junparser added a comment to D93497: Salvage debug info for function arguments in coro-split funclets..

I looked into the duplicate variables. For your first testcase, there is first the function parameter t:

ParmVarDecl 0x1585a3de8 </Users/adrian/Downloads/test/coro-debug-2.cpp:30:10, col:24> col:24 used t 'struct test &'

followed by an implicit VarDecl (thus the DW_AT_artificial attribute) for another t:

VarDecl 0x1585a8c30 </Users/adrian/Downloads/test/coro-debug-2.cpp:30:6> col:6 implicit used __coro_gro 'coro' nrvo cinit
`-CXXMemberCallExpr 0x1585a8b88 <col:6> 'coro'
  `-MemberExpr 0x1585a8b58 <col:6> '<bound member function type>' .get_return_object 0x1585a0990
    `-DeclRefExpr 0x1585a8b38 <col:6> 'std::experimental::__coroutine_traits_sfinae<coro>::promise_type':'coro::promise_type' lvalue Var 0x1585a57a8 '__promise' 'std::experimental::__coroutine_traits_sfinae<coro>::promise_type':'coro::promise_type'

The second one is produced by void EmitGroAlloca() in CGCoroutine.cpp. I'm assuming that this is the declaration of the function parameter as it is being captured for the CoroFrame function argument that is being passed to the various .resume functions.

Fri, Jan 22, 12:19 AM · Restricted Project

Wed, Jan 20

junparser added a comment to D93838: [LLVM] [SCCP] [WIP] : Add Function Specialization pass.

hi @mivnay, could you please rebase this and D93762 ? So we can test it.
It say that this patch can improve mcf 8%, we are interested at this because we also found that mcf has performance regression caused by ILP data dependency.

Wed, Jan 20, 5:15 AM · Restricted Project

Sun, Jan 17

junparser accepted D94834: [Coroutine] Do not CoroElide if there are musttail calls.

Thanks for fix this. Furthermore, we can add runtime check to see whether this pointer is current coroutine frame, and optimize them separately.

Sun, Jan 17, 10:42 PM · Restricted Project

Thu, Jan 14

junparser added a comment to D94388: [Coroutine][DebugInfo] Enhance the ability of coroutine to debug parameters under O2.

Do we understand *why* the frontend is emitting the variables twice? If that just a bug in the clang frontend (which would be my first guess) then we should aim at fixing the frontend instead. If the frontend is trying to do something different, I would love to understand what that is.

Hi @aprantl,
According to the standard, coroutine needs to copy parameters (calling constructor) passed to the coroutine function by the original caller into the coroutine frame. So in frontend, we fake local variables for this behavior, that why we see two variables in debuginfo.

Thu, Jan 14, 5:56 PM · Restricted Project
junparser added a comment to D94388: [Coroutine][DebugInfo] Enhance the ability of coroutine to debug parameters under O2.

Do we understand *why* the frontend is emitting the variables twice? If that just a bug in the clang frontend (which would be my first guess) then we should aim at fixing the frontend instead. If the frontend is trying to do something different, I would love to understand what that is.

Thu, Jan 14, 5:54 PM · Restricted Project

Sun, Jan 10

junparser added a comment to D93497: Salvage debug info for function arguments in coro-split funclets..

Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

I was hoping that in an optimization pipeline the coroutine splitting would be invoked before mem2reg. This patch is really depending on the semantics of dbg.declare, which guarantees that the address of the variable doesn't change throughout the entire function. With a dbg.value we don't have any such guarantees, since it's just a snapshot at one particular location.

Sun, Jan 10, 9:46 PM · Restricted Project

Dec 21 2020

junparser added a comment to D93497: Salvage debug info for function arguments in coro-split funclets..

Thanks for the patch! Although I may not the right person to accept the patch. BTW, Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

Dec 21 2020, 4:45 PM · Restricted Project

Dec 17 2020

junparser committed rG013839990377: [InstCombine] Remove scalable vector restriction in InstCombineCasts (authored by junparser).
[InstCombine] Remove scalable vector restriction in InstCombineCasts
Dec 17 2020, 6:03 AM
junparser closed D93389: [InstCombine] Remove scalable vector restriction in InstCombineCasts.
Dec 17 2020, 6:03 AM · Restricted Project
junparser accepted D93342: Cleanup coro-inline.ll.

LGTM

Dec 17 2020, 5:38 AM · Restricted Project
junparser added a comment to D93389: [InstCombine] Remove scalable vector restriction in InstCombineCasts.

Thanks for review the patch !

Dec 17 2020, 5:35 AM · Restricted Project

Dec 16 2020

junparser updated the diff for D93389: [InstCombine] Remove scalable vector restriction in InstCombineCasts.
Dec 16 2020, 10:04 PM · Restricted Project
junparser updated the diff for D93389: [InstCombine] Remove scalable vector restriction in InstCombineCasts.

Remove the change that do not have testcases

Dec 16 2020, 10:03 PM · Restricted Project
junparser requested review of D93389: [InstCombine] Remove scalable vector restriction in InstCombineCasts.
Dec 16 2020, 3:07 AM · Restricted Project

Dec 15 2020

junparser committed rG52a3267ffafc: [InstCombine] Remove scalable vector restriction in foldVectorBinop (authored by junparser).
[InstCombine] Remove scalable vector restriction in foldVectorBinop
Dec 15 2020, 5:15 AM
junparser closed D93289: [InstCombine] Remove scalable vector restriction in foldVectorBinop.
Dec 15 2020, 5:15 AM · Restricted Project
junparser added a comment to D93289: [InstCombine] Remove scalable vector restriction in foldVectorBinop.

LGTM with one minor

Dec 15 2020, 4:57 AM · Restricted Project
junparser committed rGe12f58457800: [InstCombine] Remove scalable vector restriction in InstCombineCompares (authored by junparser).
[InstCombine] Remove scalable vector restriction in InstCombineCompares
Dec 15 2020, 4:47 AM
junparser committed rGffe84d90e9a7: [InstCombine][NFC] Change cast of FixedVectorType to dyn_cast. (authored by junparser).
[InstCombine][NFC] Change cast of FixedVectorType to dyn_cast.
Dec 15 2020, 4:47 AM
junparser committed rG2ac58e21a115: [InstCombine] Remove scalable vector restriction when fold SelectInst (authored by junparser).
[InstCombine] Remove scalable vector restriction when fold SelectInst
Dec 15 2020, 4:47 AM
junparser closed D93269: [InstCombine] Remove scalable vector restriction in InstCombineCompares.
Dec 15 2020, 4:47 AM · Restricted Project
junparser closed D93083: [InstCombine] Remove scalable vector restriction in foldSelectOpOp.
Dec 15 2020, 4:47 AM · Restricted Project
junparser added a comment to D93289: [InstCombine] Remove scalable vector restriction in foldVectorBinop.

address the comment, update the testcase.

Dec 15 2020, 4:22 AM · Restricted Project
junparser updated the diff for D93289: [InstCombine] Remove scalable vector restriction in foldVectorBinop.
Dec 15 2020, 4:21 AM · Restricted Project
junparser added a comment to D93269: [InstCombine] Remove scalable vector restriction in InstCombineCompares.

@RKSimon Thank for review the patch. Would please take a look at the similar D93289 and D93083. Thanks a lot.

Dec 15 2020, 3:53 AM · Restricted Project
junparser added a comment to D93269: [InstCombine] Remove scalable vector restriction in InstCombineCompares.

address the comment.

Dec 15 2020, 2:54 AM · Restricted Project
junparser updated the diff for D93269: [InstCombine] Remove scalable vector restriction in InstCombineCompares.
Dec 15 2020, 2:54 AM · Restricted Project
junparser added a reviewer for D93289: [InstCombine] Remove scalable vector restriction in foldVectorBinop: RKSimon.
Dec 15 2020, 2:44 AM · Restricted Project
junparser added a reviewer for D93083: [InstCombine] Remove scalable vector restriction in foldSelectOpOp: spatel.
Dec 15 2020, 2:39 AM · Restricted Project
junparser added a reviewer for D93269: [InstCombine] Remove scalable vector restriction in InstCombineCompares: spatel.
Dec 15 2020, 2:39 AM · Restricted Project
junparser requested review of D93289: [InstCombine] Remove scalable vector restriction in foldVectorBinop.
Dec 15 2020, 2:38 AM · Restricted Project

Dec 14 2020

junparser updated the summary of D93269: [InstCombine] Remove scalable vector restriction in InstCombineCompares.
Dec 14 2020, 7:55 PM · Restricted Project
junparser requested review of D93269: [InstCombine] Remove scalable vector restriction in InstCombineCompares.
Dec 14 2020, 7:49 PM · Restricted Project

Dec 13 2020

junparser updated the diff for D93083: [InstCombine] Remove scalable vector restriction in foldSelectOpOp.

Address the comment, update the testcase. Also fix same issue in foldBitcastSelect.

Dec 13 2020, 8:18 PM · Restricted Project

Dec 10 2020

junparser requested review of D93083: [InstCombine] Remove scalable vector restriction in foldSelectOpOp.
Dec 10 2020, 7:37 PM · Restricted Project
junparser committed rG137674f882fc: [TruncInstCombine] Remove scalable vector restriction (authored by junparser).
[TruncInstCombine] Remove scalable vector restriction
Dec 10 2020, 2:00 AM
junparser closed D92819: [TruncInstCombine] Remove scalable vector restriction.
Dec 10 2020, 2:00 AM · Restricted Project
junparser added inline comments to D92819: [TruncInstCombine] Remove scalable vector restriction.
Dec 10 2020, 1:09 AM · Restricted Project

Dec 9 2020

junparser added a comment to D92819: [TruncInstCombine] Remove scalable vector restriction.

kindly ping~

Dec 9 2020, 7:37 PM · Restricted Project

Dec 8 2020

junparser requested review of D92819: [TruncInstCombine] Remove scalable vector restriction.
Dec 8 2020, 12:19 AM · Restricted Project
junparser accepted D92706: [coroutine] should disable inline before calling coro split.

hi @lxfind, sorry for the late reply. After D85812, we think it might be better to move both of attribute and intrinsic of coroutine into core library, however such changes are too heavy to do.
The reason I suggest adding the attribute into EnumAttr is that i believe that we may need to use presplit attribute in other places (such as lower debug intrinsic. ) and we do not want to write the string anywhere cross different parts. Add API in Function will reduce the impact, so LGTM, thanks for the patch.

Dec 8 2020, 12:13 AM · Restricted Project

Dec 6 2020

junparser committed rG216689ace71d: [Coroutines] Add DW_OP_deref for transformed dbg.value intrinsic. (authored by junparser).
[Coroutines] Add DW_OP_deref for transformed dbg.value intrinsic.
Dec 6 2020, 6:26 PM
junparser closed D92462: [Coroutines] Add DW_OP_deref for transformed dbg.value intrinsic..
Dec 6 2020, 6:25 PM · Restricted Project

Dec 2 2020

junparser added a comment to D92462: [Coroutines] Add DW_OP_deref for transformed dbg.value intrinsic..

Thanks for following up with this, I did miss to incorporate this suggestion! Out of curiosity, are you using lldb or gdb to print the variable? In lldb I already get values instead of address before this patch, so I wonder if one debugger needs more specific info than the other.

Dec 2 2020, 5:07 PM · Restricted Project

Dec 1 2020

junparser updated the summary of D92462: [Coroutines] Add DW_OP_deref for transformed dbg.value intrinsic..
Dec 1 2020, 10:57 PM · Restricted Project
junparser requested review of D92462: [Coroutines] Add DW_OP_deref for transformed dbg.value intrinsic..
Dec 1 2020, 10:56 PM · Restricted Project

Nov 24 2020

junparser added a comment to D85812: [coroutine] should disable inline before calling coro split.

Hi @dongAxis1944, @junparser, do you still plan to reland this patch?

Nov 24 2020, 6:11 PM · Restricted Project

Nov 14 2020

junparser accepted D91305: [Coroutine] Allocas used by StoreInst does not always escape.

Thanks for the patch!

Nov 14 2020, 5:58 PM · Restricted Project

Nov 13 2020

junparser added a comment to D91305: [Coroutine] Allocas used by StoreInst does not always escape.

I'm not sure whether we should fix this case by case. Maybe we should use memoryssa to handle this.

In general yes I agree with you that we don't want to handle case by case and make this over-complicated. However there are good reasons that this patch is quite necessary.

  1. When optimizations are enabled, there are usually pretty accurate lifetime information, and hence we don't usually even need the alloca visitor to track them. So the alloca visitor is crucial when optimizations are turned off and there is no lifetime information. When optimizations are turned off, because there isn't mem2reg, almost all data accesses are done through the patterns of stores followed with loads. What I found is that in such case, when optimizations are turned off, almost all allocas are put on the frame because they are all used in store instructions, making this a bit useless. This patch is a key to make the alloca tracking effective and so that majority of allocas can be moved out of frame. Hence the effect of this patch is quite significant comparing to the effort. It's unlikely that I will add any more logic to track other cases.
  2. This is also a continuation of D90990, which is to avoid frame-use-after-destroy. What I found is that, even I fixed the front-end such that the use of temporaries after a call to await_suspend() are contained locally that does not go across suspension points, because they are all involved in this store/load pattern, they are still being put on the frame, causing memory corruptions in non-optimized mode. This patch can fix that issue completely.
Nov 13 2020, 8:54 PM · Restricted Project

Nov 12 2020

junparser added a comment to D91305: [Coroutine] Allocas used by StoreInst does not always escape.

I'm not sure whether we should fix this case by case. Maybe we should use memoryssa to handle this.

Nov 12 2020, 7:36 PM · Restricted Project

Nov 10 2020

junparser added a comment to D90772: [Coroutines] Add missing llvm.dbg.declare's to cover more allocas.

Hi @junparser,

One of the another issue is that it is also necessary to track coroutine function parameters correctly under O1/O2 level which use dbg.value. Thoughts?

I haven't looked up the coroutine function parameter debugging quality yet, does it even work fine for -O0? I'm trying to go over the issues I find for -O0 first and incrementally improve on top of that. Any specific testcase in mind?

Nov 10 2020, 3:53 AM · Restricted Project

Nov 9 2020

junparser added a comment to D90772: [Coroutines] Add missing llvm.dbg.declare's to cover more allocas.

Hi Bruno,
One of the another issue is that it is also necessary to track coroutine function parameters correctly under O1/O2 level which use dbg.value. Thoughts?

Nov 9 2020, 5:57 PM · Restricted Project
junparser accepted D90977: [Coroutine] Move all used local allocas to the .resume function.

LGTM, Thanks!

Nov 9 2020, 5:00 PM · Restricted Project

Nov 6 2020

junparser added a comment to D90772: [Coroutines] Add missing llvm.dbg.declare's to cover more allocas.

This patch works for llvm.dbg.addr. So what is status of llvm.dbg.addr? does anyone know about it ?

Nov 6 2020, 12:15 AM · Restricted Project

Oct 28 2020

junparser added inline comments to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.
Oct 28 2020, 7:00 PM · Restricted Project
junparser added inline comments to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.
Oct 28 2020, 12:00 AM · Restricted Project

Oct 27 2020

junparser accepted D89768: [Coroutine] Properly determine whether an alloca should live on the frame.

Thanks for the patch!

Oct 27 2020, 11:44 PM · Restricted Project
junparser added inline comments to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.
Oct 27 2020, 11:36 PM · Restricted Project
junparser added inline comments to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.
Oct 27 2020, 11:16 PM · Restricted Project
junparser added inline comments to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.
Oct 27 2020, 11:05 PM · Restricted Project
junparser added inline comments to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.
Oct 27 2020, 11:01 PM · Restricted Project
junparser added inline comments to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.
Oct 27 2020, 10:52 PM · Restricted Project
junparser added inline comments to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.
Oct 27 2020, 7:27 PM · Restricted Project
junparser added a comment to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.

Thank you for the explanation!

Oct 27 2020, 7:25 PM · Restricted Project
junparser added inline comments to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.
Oct 27 2020, 8:00 AM · Restricted Project
junparser added a comment to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.

Thanks for the patch! I think I generally agree with this patch.
One thing is that the aliases seems to be over-analyzed. Can we just leave aliases on frame? Cause I have no idea about this effect on real workloads.

What do you mean by "leave aliases on frame"?

Just setEscaped for pointer cast used before coro.begin, or can we do something like llvm::PointerMayBeCapturedBefore to check the pointer directly? I'm not sure about this.

Oct 27 2020, 7:53 AM · Restricted Project

Oct 26 2020

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

Thanks for the patch! I think I generally agree with this patch.
One thing is that the aliases seems to be over-analyzed. Can we just leave aliases on frame? Cause I have no idea about this effect on real workloads.

What do you mean by "leave aliases on frame"?

Oct 26 2020, 11:36 PM · Restricted Project
junparser added a comment to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.

Thanks for the patch! I think I generally agree with this patch.
One thing is that the aliases seems to be over-analyzed. Can we just leave aliases on frame? Cause I have no idea about this effect on real workloads.

Oct 26 2020, 7:17 AM · Restricted Project

Oct 12 2020

junparser added a comment to D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter.

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

I'm not familiar with ASAN instrumentation. Do you have any testcases to explain this?

Unfortunately I don't. But this is not related to ASAN. Basically, this is causing destructing of objects that should still be alive. I suspect that it's because ExprWithCleanups always clean up temps that belongs to the full expression, not just the sub-expression in it.

Oct 12 2020, 7:02 PM · Restricted Project
junparser added a comment to D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter.

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

Oct 12 2020, 1:12 AM · Restricted Project

Oct 11 2020

junparser added a comment to D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter.

why we should not do this with normal await call?

Oct 11 2020, 7:25 PM · Restricted Project

Sep 28 2020

junparser planned changes to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

@efriedma Consider that temporary fix maybe not the best solution for such issue. We would try to find other general solution. Before that, I'll keep this patch as plan changes.

Sep 28 2020, 5:21 PM · Restricted Project
junparser added a comment to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

one of the idea is that we do not generate default case for switch instruction of coro.suspend intrinsic. Instead, we find fall through coro.end for coro.suspend directly in corosplit pass. The only uncertain thing is whether fall through coro.end is moved by passes which may change the semantic.

@lxfind, what do you think about this idea?

Are you suggesting to not have an edge from the coro.suspend switch to the coro.end block? Would that make the coro.end BB a dead BB and could potentially get removed?

No, There is always path from entry to coro.end BB.

This is likely the right solution, but we do need to make it more general. In order for this to always work, I wonder if the proper place to insert this transformation is in LoopSimplify, which is a pass always executed before any Loop pass. That might guarantee that any loop pass will be able to handle this properly. I am not too familiar with loop passes, but maybe someone else could comment on that.

Loop exit-block insertion in LoopSimplify pass guarantees that all exit blocks from the loop only have predecessors from inside of the loop. It will break the dedicated form of exit blocks when we consider ignore coro.suspend path. Maybe there is other idea i haven't thought about.

I wasn't referring to ignoring the coro.suspend path, but to move this code in this patch into the LoopSimplify pass, so that all loop passes can benefit, not sure if that makes sense. Anyway, I don't have a better idea.. So don't let that block you here.

Sep 28 2020, 5:12 PM · Restricted Project

Sep 27 2020

junparser accepted D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.

LGTM, thanks for the patch!

Sep 27 2020, 11:00 PM · Restricted Project

Sep 26 2020

junparser added inline comments to D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes.
Sep 26 2020, 9:13 PM · Restricted Project

Sep 24 2020

junparser added a comment to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

one of the idea is that we do not generate default case for switch instruction of coro.suspend intrinsic. Instead, we find fall through coro.end for coro.suspend directly in corosplit pass. The only uncertain thing is whether fall through coro.end is moved by passes which may change the semantic.

@lxfind, what do you think about this idea?

Are you suggesting to not have an edge from the coro.suspend switch to the coro.end block? Would that make the coro.end BB a dead BB and could potentially get removed?

No, There is always path from entry to coro.end BB.

Sep 24 2020, 6:44 PM · Restricted Project

Sep 23 2020

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

Please remove unnecessary headers include and declarations

Sep 23 2020, 8:28 PM · Restricted Project

Sep 22 2020

junparser added a comment to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

one of the idea is that we do not generate default case for switch instruction of coro.suspend intrinsic. Instead, we find fall through coro.end for coro.suspend directly in corosplit pass. The only uncertain thing is whether fall through coro.end is moved by passes which may change the semantic.

Sep 22 2020, 11:27 PM · Restricted Project
junparser added a comment to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

Can you please extend the testing to cover at least some of these cases: multiple predecesssors, multiple exit blocks, multiple predecessors with a coroutine switch?

Sep 22 2020, 8:12 PM · Restricted Project
junparser added inline comments to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..
Sep 22 2020, 8:08 PM · Restricted Project

Sep 21 2020

junparser updated the diff for D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..
Sep 21 2020, 7:45 PM · Restricted Project
junparser updated the diff for D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

Update the doc. @efriedma is this ok?

Sep 21 2020, 7:30 PM · Restricted Project
junparser added a comment to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

sorry for the late reply.

Sep 21 2020, 7:05 PM · Restricted Project

Sep 18 2020

junparser added a comment to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

If there are transformations that are specifically illegal with corouties, I'd like to see a section in https://llvm.org/docs/Coroutines.html describing what transforms are and are not legal. Just saying that the current version of the coroutine lowering code can't handle it isn't sufficient.

Sep 18 2020, 1:15 AM · Restricted Project

Sep 17 2020

junparser accepted D87810: [Coroutine] Fix a bug where Coroutine incorrectly spills phi and invoke defs before CoroBegin.

LGTM, Thanks for the fix.

Sep 17 2020, 3:11 AM · Restricted Project
junparser added a comment to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

Sep 17 2020, 2:43 AM · Restricted Project
junparser added a comment to D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

Sep 17 2020, 1:56 AM · Restricted Project
junparser requested review of D87817: [LICM][Coroutine] Don't sink stores to coroutine suspend point..
Sep 17 2020, 1:44 AM · Restricted Project

Sep 15 2020

junparser added a reviewer for D87731: [Coro][NewPM] Handle llvm.coro.prepare.retcon in NPM coro-split pass: rjmccall.
Sep 15 2020, 7:37 PM · Restricted Project
junparser added a comment to D87731: [Coro][NewPM] Handle llvm.coro.prepare.retcon in NPM coro-split pass.

what does this intrinsic use for?

Sep 15 2020, 7:34 PM · Restricted Project
junparser added a comment to D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle.

@lxfind , could you backport this to branch 11?

Sep 15 2020, 12:44 AM · Restricted Project

Sep 14 2020

junparser accepted D83563: [Coroutines] Fix a typo in documentation.

LGTM, thanks!

Sep 14 2020, 6:55 PM · Restricted Project

Sep 10 2020

junparser added a comment to D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle.

Thanks for the change. LGTM, and testcase?

Sep 10 2020, 6:46 PM · Restricted Project

Aug 24 2020

junparser abandoned D81549: [LVI] Make use of 'assume'-provided data-part1.
Aug 24 2020, 1:46 AM · Restricted Project

Aug 23 2020

junparser abandoned D86190: [LICM][Coroutine] Don't sink stores from loops with coro.suspend instructions..
Aug 23 2020, 6:36 PM · Restricted Project

Aug 19 2020

junparser added inline comments to D86190: [LICM][Coroutine] Don't sink stores from loops with coro.suspend instructions..
Aug 19 2020, 8:57 PM · Restricted Project
junparser added a comment to D86190: [LICM][Coroutine] Don't sink stores from loops with coro.suspend instructions..

Are you sure this is the right fix? At first glance, ensuring %p lives long enough seems like the sort of thing coroutine lowering should be able to deal with. If it isn't, at the very least we need documentation describing what optimizations are legal.

Aug 19 2020, 8:24 PM · Restricted Project

Aug 18 2020

junparser added a reviewer for D86190: [LICM][Coroutine] Don't sink stores from loops with coro.suspend instructions.: modocache.
Aug 18 2020, 11:13 PM · Restricted Project
junparser updated the summary of D86190: [LICM][Coroutine] Don't sink stores from loops with coro.suspend instructions..
Aug 18 2020, 8:01 PM · Restricted Project
junparser requested review of D86190: [LICM][Coroutine] Don't sink stores from loops with coro.suspend instructions..
Aug 18 2020, 8:00 PM · Restricted Project