Page MenuHomePhabricator

junparser (JunMa)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 11 2019, 11:18 PM (48 w, 6 d)

Recent Activity

Jan 16 2020

junparser added a comment to D71663: [Coroutines] CoroElide enhancement .

gental ping~

Jan 16 2020, 6:24 PM · Restricted Project

Jan 5 2020

junparser added a comment to D71826: [Coroutines] Remove corresponding phi values when apply simplifyTerminatorLeadingToRet.

Done! Please excuse the wait. Have you considered getting commit access to LLVM? You won't have to wait on me in the future :) https://llvm.org/docs/DeveloperPolicy.html#new-contributors

Jan 5 2020, 5:49 PM · Restricted Project

Jan 4 2020

junparser added a comment to D71663: [Coroutines] CoroElide enhancement .

@modocache kindly ping~

Jan 4 2020, 2:56 AM · Restricted Project
junparser added a comment to D71903: [Coroutines][6/6] Clang schedules new passes.

I'm currently working on ensuring that CGSCC optimizations are rerun to optimize coroutine funclets -- the primary feedback I received on this and on D71899 -- but I just realized I didn't respond to one comment on this set of reviews, from @junparser:

There is another issue we should consider: clang is crashed when compile coroutine with -disable-llvm-passes and output an object file.

It's always been the case, since the coroutine intrinsics and passes were first added to LLVM, that attempting to codegen without first running coroutine passes would cause a crash during instruction selection. So clang -Xclang -disable-llvm-passes -c has always crashed Clang during LLVM ISel, as it does in this example that uses Clang 9.0.0 and the legacy pass manager: https://godbolt.org/z/Mj2R5G

Personally I'm of the opinion that this is less than ideal... I may be wrong, but I don't think there are very many other C++ features that *require* Clang to run LLVM passes (perhaps the always_inline attribute requires LLVM passes to be run for correctness? I'm not sure). So I would like to see this eventually addressed somehow.

Is it reasonable to run coroutine passes even -disable-llvm-passes is enabled?

My personal opinion is that this would not be reasonable. The option -disable-llvm-passes should, from my point of view, prevent any and all LLVM passes from being run. I also frequently make use of this option when debugging the LLVM IR being output for C++ coroutines code, so if -disable-llvm-passes didn't disable coroutines passes, I'd need another option that did (-disable-llvm-passes-no-really-even-coroutine-passes-them-too 😅).

All this being said, considering this behavior has existed in the legacy PM since day one, I think we should start a separate discussion on if/how to change that behavior. I'm working on an update for these patches to address funclet optimization, but the update will not change the fact that coroutine passes are not run when -disable-llvm-passes is specified. I think that's an orthogonal issue.

Jan 4 2020, 2:43 AM · Restricted Project

Dec 30 2019

junparser added a comment to D71899: [Coroutines][2/6] New pass manager: coro-split.

The devirt trigger here is to restart CGSCC pipeline to run coro-split again to split the coroutine function. There is no need to introduce devirt trigger in NewPM since we call coro-split pass manually.

Manually schedule the 2nd coro-split pass is only a workaround before we can trigger CGSCC passes on the split funclet like we did for legacy PM. It does the split without restarting CGSCC passes so it works, but it also leaves performance on the table because the split funclets won't go through many opt passes of CGSCC pipeline. Yes, I agree don't need to introduce devirt trigger with new PM, but that's because I think we can request CGSCC passes on split funclet via other mechanism like CGSCCUpdateResult, not just because 2nd coro-split pass is manually scheduled.

There are two issues here: 1) coro-split needs run at least twice, we do not need CGSCC pipeline at pre-split stage which coro-split pass just works as a function pass 2) request CGSCC passes on split funclet after 2nd running of coro-split, and coroutine optimization such as coro-elide pass also depends on these optimization.
Manually schedule coro-split twice is a workaround for 1), as for 2) the current pipeline of coroutine in this patch set need be changed.

Dec 30 2019, 10:52 PM · Restricted Project
junparser added a comment to D71899: [Coroutines][2/6] New pass manager: coro-split.

My understanding is devirt trigger is only a trick used with Legacy PM to run optimizations on coroutine funclets after coro-split. With NewPM's CGSCCUpdateResult infra, can we communicate that change and request passes directly with ModuleToPostOrderCGSCCPassAdaptor, without artificially introducing the indirect call and devirt?

The devirt trigger here is to restart CGSCC pipeline to run coro-split again to split the coroutine function. There is no need to introduce devirt trigger in NewPM since we call coro-split pass manually.

Dec 30 2019, 7:55 PM · Restricted Project

Dec 29 2019

junparser updated the diff for D71826: [Coroutines] Remove corresponding phi values when apply simplifyTerminatorLeadingToRet.

update the testcase, and @modocache can you commit this patch? thanks.

Dec 29 2019, 6:46 PM · Restricted Project
junparser added a comment to D71903: [Coroutines][6/6] Clang schedules new passes.

There is another issue we should consider: clang is crashed when compile coroutine with -disable-llvm-passes and output an object file.
Is it reasonable to run coroutine passes even -disable-llvm-passes is enabled?

Dec 29 2019, 6:10 PM · Restricted Project
junparser added a comment to D71826: [Coroutines] Remove corresponding phi values when apply simplifyTerminatorLeadingToRet.

Sorry for the wait here, @junparser! I wanted to send my own patches out before looking at this one 😛

This is really nice, thanks for the change! As per your test plan, I confirmed that with this patch, cppcoro issue https://github.com/lewissbaker/cppcoro/issues/109 no longer reproduces for me. The jump threading issue you describe must have been responsible for Clang hanging indefinitely when compiling that test file.

I had one nitpick related to your test, please address it or not at your own discretion. This patch LGTM.

Dec 29 2019, 6:01 PM · Restricted Project
junparser added a comment to D71663: [Coroutines] CoroElide enhancement .

@modocache kindly ping~

Dec 29 2019, 6:01 PM · Restricted Project

Dec 26 2019

junparser added inline comments to D71903: [Coroutines][6/6] Clang schedules new passes.
Dec 26 2019, 9:40 PM · Restricted Project
junparser added a comment to D71826: [Coroutines] Remove corresponding phi values when apply simplifyTerminatorLeadingToRet.

@modocache kindly ping~

Dec 26 2019, 3:37 AM · Restricted Project

Dec 22 2019

junparser added a comment to D71663: [Coroutines] CoroElide enhancement .

kindly ping~

Dec 22 2019, 11:20 PM · Restricted Project
junparser created D71826: [Coroutines] Remove corresponding phi values when apply simplifyTerminatorLeadingToRet.
Dec 22 2019, 11:20 PM · Restricted Project

Dec 18 2019

junparser updated the summary of D71663: [Coroutines] CoroElide enhancement .
Dec 18 2019, 7:11 AM · Restricted Project
junparser created D71663: [Coroutines] CoroElide enhancement .
Dec 18 2019, 7:11 AM · Restricted Project

Nov 28 2019

junparser added a comment to D70579: [coroutines][PR41909] Generalize fix from D62550.

This also fix same ice when build cppcoro with current trunk. FYI

Nov 28 2019, 5:48 PM · Restricted Project

Nov 22 2019

junparser added a comment to D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves.

LGTM, thanks! Please let me know if you'd like me to commit this change.

Nov 22 2019, 2:46 AM · Restricted Project

Nov 21 2019

junparser added a comment to D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves.

Sorry for the slow response here, @junparser!

The test case you came up with here is great! I can see the issue is that ScopeInfo->CoroutineParameterMoves are built up when Clang parses the first co_await a, but are not cleared when lookupPromiseType results in an error. As a result, when Clang hits the second co_await a, it's in a state that the current code didn't anticipate. Your test case does a great job demonstrating this. Your fix for the problem also looks good to me! My only suggestion is to make the test case just a little clearer, as I'll explain...

(Also, in the future could you please upload your patches with full context? You can read https://llvm.org/docs/Phabricator.html for more details. I think the section explaining the web interface might be relevant to you, where it suggests git show HEAD -U999999 > mypatch.patch. The reason I ask is because on Phabricator I can see what lines you're proposing should be added, but not the surrounding source lines, which made this more difficult to review.)

Nov 21 2019, 7:39 PM · Restricted Project
junparser updated the diff for D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves.

update as @modocache's suggestion

Nov 21 2019, 7:30 PM · Restricted Project

Oct 25 2019

junparser updated subscribers of D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves.

Despite generally knowing about coroutines and generally knowing about Clang, I actually don't know the Clang coroutine code and can't review this properly.

Oct 25 2019, 9:02 AM · Restricted Project

Oct 24 2019

junparser added a reviewer for D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves: rjmccall.
Oct 24 2019, 11:32 AM · Restricted Project

Oct 21 2019

junparser added a comment to D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves.

gental ping~ @modocache @GorNishanov

Oct 21 2019, 8:51 AM · Restricted Project

Oct 16 2019

junparser created D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves.
Oct 16 2019, 12:59 AM · Restricted Project