[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-30 Thread Yuanfang Chen via Phabricator via cfe-commits 
ychen added inline comments.



Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2001
 
-if ((Shape.ABI == coro::ABI::Async || Shape.ABI == coro::ABI::Retcon ||
- Shape.ABI == coro::ABI::RetconOnce) &&
-!Shape.CoroSuspends.empty()) {
-  // Run the CGSCC pipeline on the newly split functions.
-  // All clones will be in the same RefSCC, so choose a random clone.
-  UR.RCWorklist.insert(CG.lookupRefSCC(CG.get(*Clones[0])));
+if (!Shape.CoroSuspends.empty()) {
+  // Run the CGSCC pipeline on the original and newly split functions.

lxfind wrote:
> ychen wrote:
> > aeubanks wrote:
> > > ChuanqiXu wrote:
> > > > I am not familiar with the Shape.ABI other than coro::ABI:switch. But 
> > > > the diff line seems strange, it looks like that condition gets weaker.
> > > I believe that's intentional, and a big part of this patch. We want to 
> > > re-add the current SCC (and the split SCCs) any time we split an SCC. 
> > > Before we weren't properly doing that.
> > I got your point. So "// All clones will be in the same RefSCC " : this 
> > is not accurate I think?
> Note that previously this is done only for Async, Retcon and RetconOnce ABIs, 
> not for the Switch ABI.
> I guess that's accurate for those ABIs? But for Switch ABI this is not true.
> And before we were not adding back the split functions to the pipeline to be 
> properly optimized. Now we are dong that. This should help improve the 
> performance of the post-split functions.
I missed the ABI condition. Thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-30 Thread Xun Li via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG822b92aae439: [Coroutines] Add the newly generated SCCs back 
to the CGSCC work queue after… (authored by lxfind).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

Files:
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Transforms/Coroutines/CoroSplit.cpp
  llvm/test/Transforms/Coroutines/ArgAddr.ll
  llvm/test/Transforms/Coroutines/coro-alloc-with-param-O0.ll
  llvm/test/Transforms/Coroutines/coro-alloc-with-param-O2.ll
  llvm/test/Transforms/Coroutines/coro-alloca-01.ll
  llvm/test/Transforms/Coroutines/coro-alloca-02.ll
  llvm/test/Transforms/Coroutines/coro-alloca-03.ll
  llvm/test/Transforms/Coroutines/coro-alloca-04.ll
  llvm/test/Transforms/Coroutines/coro-alloca-05.ll
  llvm/test/Transforms/Coroutines/coro-alloca-06.ll
  llvm/test/Transforms/Coroutines/coro-alloca-07.ll
  llvm/test/Transforms/Coroutines/coro-alloca-08.ll
  llvm/test/Transforms/Coroutines/coro-async.ll
  llvm/test/Transforms/Coroutines/coro-byval-param.ll
  llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll
  llvm/test/Transforms/Coroutines/coro-catchswitch.ll
  llvm/test/Transforms/Coroutines/coro-debug.ll
  llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-00.ll
  llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-01.ll
  llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-02.ll
  llvm/test/Transforms/Coroutines/coro-frame-arrayalloca.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-00.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-01.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-02.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-03.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-04.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-05.ll
  llvm/test/Transforms/Coroutines/coro-frame-unreachable.ll
  llvm/test/Transforms/Coroutines/coro-frame.ll
  llvm/test/Transforms/Coroutines/coro-materialize.ll
  llvm/test/Transforms/Coroutines/coro-padding.ll
  llvm/test/Transforms/Coroutines/coro-param-copy.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca.ll
  llvm/test/Transforms/Coroutines/coro-retcon-frame.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value2.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values2.ll
  llvm/test/Transforms/Coroutines/coro-retcon-unreachable.ll
  llvm/test/Transforms/Coroutines/coro-retcon-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon.ll
  llvm/test/Transforms/Coroutines/coro-spill-after-phi.ll
  llvm/test/Transforms/Coroutines/coro-spill-corobegin.ll
  llvm/test/Transforms/Coroutines/coro-spill-defs-before-corobegin.ll
  llvm/test/Transforms/Coroutines/coro-spill-promise.ll
  llvm/test/Transforms/Coroutines/coro-split-00.ll
  llvm/test/Transforms/Coroutines/coro-split-02.ll
  llvm/test/Transforms/Coroutines/coro-split-alloc.ll
  llvm/test/Transforms/Coroutines/coro-split-dbg.ll
  llvm/test/Transforms/Coroutines/coro-split-eh-00.ll
  llvm/test/Transforms/Coroutines/coro-split-eh-01.ll
  llvm/test/Transforms/Coroutines/coro-split-hidden.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail1.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail2.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail3.ll
  llvm/test/Transforms/Coroutines/coro-split-recursive.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-01.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-02.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-03.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-04.ll
  llvm/test/Transforms/Coroutines/coro-swifterror.ll
  llvm/test/Transforms/Coroutines/coro-zero-alloca.ll
  llvm/test/Transforms/Coroutines/no-suspend.ll
  llvm/test/Transforms/Coroutines/restart-trigger.ll
  llvm/test/Transforms/Coroutines/smoketest.ll

Index: llvm/test/Transforms/Coroutines/smoketest.ll
===
--- llvm/test/Transforms/Coroutines/smoketest.ll
+++ llvm/test/Transforms/Coroutines/smoketest.ll
@@ -10,12 +10,16 @@
 ; RUN: opt < %s -disable-output -passes='default' -enable-coroutines \
 ; RUN: -debug-pass-manager 2>&1 | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-OPT
 ; RUN: opt < %s -disable-output -debug-pass-manager \
-; RUN: -passes='function(coro-early),cgscc(coro-split),function(coro-elide,coro-cleanup)' 2>&1 \
+; RUN: -passes='function(coro-early),function(coro-elide),cgscc(coro-split),function(coro-cleanup)' 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-OPT
 
+; note that we

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-30 Thread Xun Li via Phabricator via cfe-commits 
lxfind added inline comments.



Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2001
 
-if ((Shape.ABI == coro::ABI::Async || Shape.ABI == coro::ABI::Retcon ||
- Shape.ABI == coro::ABI::RetconOnce) &&
-!Shape.CoroSuspends.empty()) {
-  // Run the CGSCC pipeline on the newly split functions.
-  // All clones will be in the same RefSCC, so choose a random clone.
-  UR.RCWorklist.insert(CG.lookupRefSCC(CG.get(*Clones[0])));
+if (!Shape.CoroSuspends.empty()) {
+  // Run the CGSCC pipeline on the original and newly split functions.

ychen wrote:
> aeubanks wrote:
> > ChuanqiXu wrote:
> > > I am not familiar with the Shape.ABI other than coro::ABI:switch. But the 
> > > diff line seems strange, it looks like that condition gets weaker.
> > I believe that's intentional, and a big part of this patch. We want to 
> > re-add the current SCC (and the split SCCs) any time we split an SCC. 
> > Before we weren't properly doing that.
> I got your point. So "// All clones will be in the same RefSCC " : this 
> is not accurate I think?
Note that previously this is done only for Async, Retcon and RetconOnce ABIs, 
not for the Switch ABI.
I guess that's accurate for those ABIs? But for Switch ABI this is not true.
And before we were not adding back the split functions to the pipeline to be 
properly optimized. Now we are dong that. This should help improve the 
performance of the post-split functions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-30 Thread Yuanfang Chen via Phabricator via cfe-commits 
ychen added inline comments.



Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2115
+  << "\n");
 F.removeFnAttr(CORO_PRESPLIT_ATTR);
 

drive-by nit: would it be better to move it up near `Coroutines.push_back();`?



Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2001
 
-if ((Shape.ABI == coro::ABI::Async || Shape.ABI == coro::ABI::Retcon ||
- Shape.ABI == coro::ABI::RetconOnce) &&
-!Shape.CoroSuspends.empty()) {
-  // Run the CGSCC pipeline on the newly split functions.
-  // All clones will be in the same RefSCC, so choose a random clone.
-  UR.RCWorklist.insert(CG.lookupRefSCC(CG.get(*Clones[0])));
+if (!Shape.CoroSuspends.empty()) {
+  // Run the CGSCC pipeline on the original and newly split functions.

aeubanks wrote:
> ChuanqiXu wrote:
> > I am not familiar with the Shape.ABI other than coro::ABI:switch. But the 
> > diff line seems strange, it looks like that condition gets weaker.
> I believe that's intentional, and a big part of this patch. We want to re-add 
> the current SCC (and the split SCCs) any time we split an SCC. Before we 
> weren't properly doing that.
I got your point. So "// All clones will be in the same RefSCC " : this is 
not accurate I think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-30 Thread Xun Li via Phabricator via cfe-commits 
lxfind updated this revision to Diff 355607.
lxfind added a comment.

fix warning


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

Files:
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Transforms/Coroutines/CoroSplit.cpp
  llvm/test/Transforms/Coroutines/ArgAddr.ll
  llvm/test/Transforms/Coroutines/coro-alloc-with-param-O0.ll
  llvm/test/Transforms/Coroutines/coro-alloc-with-param-O2.ll
  llvm/test/Transforms/Coroutines/coro-alloca-01.ll
  llvm/test/Transforms/Coroutines/coro-alloca-02.ll
  llvm/test/Transforms/Coroutines/coro-alloca-03.ll
  llvm/test/Transforms/Coroutines/coro-alloca-04.ll
  llvm/test/Transforms/Coroutines/coro-alloca-05.ll
  llvm/test/Transforms/Coroutines/coro-alloca-06.ll
  llvm/test/Transforms/Coroutines/coro-alloca-07.ll
  llvm/test/Transforms/Coroutines/coro-alloca-08.ll
  llvm/test/Transforms/Coroutines/coro-async.ll
  llvm/test/Transforms/Coroutines/coro-byval-param.ll
  llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll
  llvm/test/Transforms/Coroutines/coro-catchswitch.ll
  llvm/test/Transforms/Coroutines/coro-debug.ll
  llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-00.ll
  llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-01.ll
  llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-02.ll
  llvm/test/Transforms/Coroutines/coro-frame-arrayalloca.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-00.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-01.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-02.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-03.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-04.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-05.ll
  llvm/test/Transforms/Coroutines/coro-frame-unreachable.ll
  llvm/test/Transforms/Coroutines/coro-frame.ll
  llvm/test/Transforms/Coroutines/coro-materialize.ll
  llvm/test/Transforms/Coroutines/coro-padding.ll
  llvm/test/Transforms/Coroutines/coro-param-copy.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca.ll
  llvm/test/Transforms/Coroutines/coro-retcon-frame.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value2.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values2.ll
  llvm/test/Transforms/Coroutines/coro-retcon-unreachable.ll
  llvm/test/Transforms/Coroutines/coro-retcon-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon.ll
  llvm/test/Transforms/Coroutines/coro-spill-after-phi.ll
  llvm/test/Transforms/Coroutines/coro-spill-corobegin.ll
  llvm/test/Transforms/Coroutines/coro-spill-defs-before-corobegin.ll
  llvm/test/Transforms/Coroutines/coro-spill-promise.ll
  llvm/test/Transforms/Coroutines/coro-split-00.ll
  llvm/test/Transforms/Coroutines/coro-split-02.ll
  llvm/test/Transforms/Coroutines/coro-split-alloc.ll
  llvm/test/Transforms/Coroutines/coro-split-dbg.ll
  llvm/test/Transforms/Coroutines/coro-split-eh-00.ll
  llvm/test/Transforms/Coroutines/coro-split-eh-01.ll
  llvm/test/Transforms/Coroutines/coro-split-hidden.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail1.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail2.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail3.ll
  llvm/test/Transforms/Coroutines/coro-split-recursive.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-01.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-02.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-03.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-04.ll
  llvm/test/Transforms/Coroutines/coro-swifterror.ll
  llvm/test/Transforms/Coroutines/coro-zero-alloca.ll
  llvm/test/Transforms/Coroutines/no-suspend.ll
  llvm/test/Transforms/Coroutines/restart-trigger.ll
  llvm/test/Transforms/Coroutines/smoketest.ll

Index: llvm/test/Transforms/Coroutines/smoketest.ll
===
--- llvm/test/Transforms/Coroutines/smoketest.ll
+++ llvm/test/Transforms/Coroutines/smoketest.ll
@@ -10,12 +10,16 @@
 ; RUN: opt < %s -disable-output -passes='default' -enable-coroutines \
 ; RUN: -debug-pass-manager 2>&1 | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-OPT
 ; RUN: opt < %s -disable-output -debug-pass-manager \
-; RUN: -passes='function(coro-early),cgscc(coro-split),function(coro-elide,coro-cleanup)' 2>&1 \
+; RUN: -passes='function(coro-early),function(coro-elide),cgscc(coro-split),function(coro-cleanup)' 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-OPT
 
+; note that we run CoroElidePass before CoroSplitPass. This is because CoroElidePass is part of
+; function simplification pipeline, which runs before CoroSplitPass. And since @foo is not
+; a

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-30 Thread Xun Li via Phabricator via cfe-commits 
lxfind added inline comments.



Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2112-2114
 StringRef Value = Attr.getValueAsString();
 LLVM_DEBUG(dbgs() << "CoroSplit: Processing coroutine '" << F.getName()
   << "' state: " << Value << "\n");

ChuanqiXu wrote:
> Refactor this into:
> ```
> LLVM_DEBUG(dbgs() << "CoroSplit: Processing coroutine '" << F.getName()
>   << "' state: " << Attr.getValueAsString() << "\n");
> ```
> could erase an warning in release build.
Good catch. How did you catch this? It seems like I don't see warning on my Mac 
by default.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-30 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks accepted this revision.
aeubanks added a comment.

The CoroSplit/PassBuilder changes lgtm


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-30 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu added inline comments.



Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2112-2114
 StringRef Value = Attr.getValueAsString();
 LLVM_DEBUG(dbgs() << "CoroSplit: Processing coroutine '" << F.getName()
   << "' state: " << Value << "\n");

Refactor this into:
```
LLVM_DEBUG(dbgs() << "CoroSplit: Processing coroutine '" << F.getName()
  << "' state: " << Attr.getValueAsString() << "\n");
```
could erase an warning in release build.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-29 Thread Xun Li via Phabricator via cfe-commits 
lxfind updated this revision to Diff 355431.
lxfind added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

Files:
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Transforms/Coroutines/CoroSplit.cpp
  llvm/test/Transforms/Coroutines/ArgAddr.ll
  llvm/test/Transforms/Coroutines/coro-alloc-with-param-O0.ll
  llvm/test/Transforms/Coroutines/coro-alloc-with-param-O2.ll
  llvm/test/Transforms/Coroutines/coro-alloca-01.ll
  llvm/test/Transforms/Coroutines/coro-alloca-02.ll
  llvm/test/Transforms/Coroutines/coro-alloca-03.ll
  llvm/test/Transforms/Coroutines/coro-alloca-04.ll
  llvm/test/Transforms/Coroutines/coro-alloca-05.ll
  llvm/test/Transforms/Coroutines/coro-alloca-06.ll
  llvm/test/Transforms/Coroutines/coro-alloca-07.ll
  llvm/test/Transforms/Coroutines/coro-alloca-08.ll
  llvm/test/Transforms/Coroutines/coro-async.ll
  llvm/test/Transforms/Coroutines/coro-byval-param.ll
  llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll
  llvm/test/Transforms/Coroutines/coro-catchswitch.ll
  llvm/test/Transforms/Coroutines/coro-debug.ll
  llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-00.ll
  llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-01.ll
  llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-02.ll
  llvm/test/Transforms/Coroutines/coro-frame-arrayalloca.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-00.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-01.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-02.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-03.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-04.ll
  llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-05.ll
  llvm/test/Transforms/Coroutines/coro-frame-unreachable.ll
  llvm/test/Transforms/Coroutines/coro-frame.ll
  llvm/test/Transforms/Coroutines/coro-materialize.ll
  llvm/test/Transforms/Coroutines/coro-padding.ll
  llvm/test/Transforms/Coroutines/coro-param-copy.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca.ll
  llvm/test/Transforms/Coroutines/coro-retcon-frame.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value2.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values2.ll
  llvm/test/Transforms/Coroutines/coro-retcon-unreachable.ll
  llvm/test/Transforms/Coroutines/coro-retcon-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon.ll
  llvm/test/Transforms/Coroutines/coro-spill-after-phi.ll
  llvm/test/Transforms/Coroutines/coro-spill-corobegin.ll
  llvm/test/Transforms/Coroutines/coro-spill-defs-before-corobegin.ll
  llvm/test/Transforms/Coroutines/coro-spill-promise.ll
  llvm/test/Transforms/Coroutines/coro-split-00.ll
  llvm/test/Transforms/Coroutines/coro-split-02.ll
  llvm/test/Transforms/Coroutines/coro-split-alloc.ll
  llvm/test/Transforms/Coroutines/coro-split-dbg.ll
  llvm/test/Transforms/Coroutines/coro-split-eh-00.ll
  llvm/test/Transforms/Coroutines/coro-split-eh-01.ll
  llvm/test/Transforms/Coroutines/coro-split-hidden.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail1.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail2.ll
  llvm/test/Transforms/Coroutines/coro-split-musttail3.ll
  llvm/test/Transforms/Coroutines/coro-split-recursive.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-01.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-02.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-03.ll
  llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-04.ll
  llvm/test/Transforms/Coroutines/coro-swifterror.ll
  llvm/test/Transforms/Coroutines/coro-zero-alloca.ll
  llvm/test/Transforms/Coroutines/no-suspend.ll
  llvm/test/Transforms/Coroutines/restart-trigger.ll
  llvm/test/Transforms/Coroutines/smoketest.ll

Index: llvm/test/Transforms/Coroutines/smoketest.ll
===
--- llvm/test/Transforms/Coroutines/smoketest.ll
+++ llvm/test/Transforms/Coroutines/smoketest.ll
@@ -10,12 +10,16 @@
 ; RUN: opt < %s -disable-output -passes='default' -enable-coroutines \
 ; RUN: -debug-pass-manager 2>&1 | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-OPT
 ; RUN: opt < %s -disable-output -debug-pass-manager \
-; RUN: -passes='function(coro-early),cgscc(coro-split),function(coro-elide,coro-cleanup)' 2>&1 \
+; RUN: -passes='function(coro-early),function(coro-elide),cgscc(coro-split),function(coro-cleanup)' 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-OPT
 
+; note that we run CoroElidePass before CoroSplitPass. This is because CoroElidePass is part of
+; function simplification pipeline, which runs before

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-29 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu added a comment.

In D95807#2849128 , @lxfind wrote:

>> 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?
>
> Ramp function will eventually be inlined, but not when you run Inliner on the 
> inlinee.
> Let's say coroutine A calls coroutine B, and eventually we want to inline B 
> into A so that we could perform CoroElide on A.
> After B is split, we don't need to run inliner again on B. When we run 
> inliner on A, A will inline B.

Thanks for clarifying.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-29 Thread Xun Li via Phabricator via cfe-commits 
lxfind added a comment.

> 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?

Ramp function will eventually be inlined, but not when you run Inliner on the 
inlinee.
Let's say coroutine A calls coroutine B, and eventually we want to inline B 
into A so that we could perform CoroElide on A.
After B is split, we don't need to run inliner again on B. When we run inliner 
on A, A will inline B.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-29 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu added a comment.

In D95807#2849120 , @lxfind wrote:

> 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
>
> Good point. I was trying to avoid the second inliner on the coroutine ramp 
> function. But I guess the cost will be bigger than the win.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-29 Thread Xun Li via Phabricator via cfe-commits 
lxfind added a comment.

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

Good point. I was trying to avoid the second inliner on the coroutine ramp 
function. But I guess the cost will be bigger than the win.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-29 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-29 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

In D95807#2847449 , @lxfind wrote:

> 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?

Since we had prevent inlining coroutine before split, coroutine can't be 
inlined pre-split. After splitting, due to the SCC changes, the inliner would 
run again for ramp.

LGTM. But I am not familiar with the pipeline, please wait for 1~2 days in case 
there are more comments.




Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2112-2114
 StringRef Value = Attr.getValueAsString();
 LLVM_DEBUG(dbgs() << "CoroSplit: Processing coroutine '" << F.getName()
   << "' state: " << Value << "\n");

These are not needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-29 Thread Xun Li via Phabricator via cfe-commits 
lxfind added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

2021-06-29 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu added a comment.

> 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.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95807/new/

https://reviews.llvm.org/D95807

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits