Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC][Coroutines] Remove redundant checks for replacing PrepareFns #98392

Merged

Conversation

yuxuanchen1997
Copy link
Member

If Coroutines.empty() the following loop is going to be skipped entirely and same goes for PrepareFns.empty(). These two conditions here aren't useful and adds to complexity.

@llvmbot
Copy link
Member

llvmbot commented Jul 10, 2024

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-coroutines

Author: Yuxuan Chen (yuxuanchen1997)

Changes

If Coroutines.empty() the following loop is going to be skipped entirely and same goes for PrepareFns.empty(). These two conditions here aren't useful and adds to complexity.


Full diff: https://github.com/llvm/llvm-project/pull/98392.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/Coroutines/CoroSplit.cpp (-8)
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index 299b514d34f1c..0b52d1e4490cb 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -2108,12 +2108,6 @@ PreservedAnalyses CoroSplitPass::run(LazyCallGraph::SCC &C,
   if (Coroutines.empty() && PrepareFns.empty())
     return PreservedAnalyses::all();
 
-  if (Coroutines.empty()) {
-    for (auto *PrepareFn : PrepareFns) {
-      replaceAllPrepares(PrepareFn, CG, C);
-    }
-  }
-
   // Split all the coroutines.
   for (LazyCallGraph::Node *N : Coroutines) {
     Function &F = N->getFunction();
@@ -2143,11 +2137,9 @@ PreservedAnalyses CoroSplitPass::run(LazyCallGraph::SCC &C,
     }
   }
 
-  if (!PrepareFns.empty()) {
     for (auto *PrepareFn : PrepareFns) {
       replaceAllPrepares(PrepareFn, CG, C);
     }
-  }
 
   return PreservedAnalyses::none();
 }

@@ -2108,12 +2108,6 @@ PreservedAnalyses CoroSplitPass::run(LazyCallGraph::SCC &C,
if (Coroutines.empty() && PrepareFns.empty())
return PreservedAnalyses::all();

if (Coroutines.empty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change NFC?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the next lines reads "for each coroutine in Coroutines". It's the same condition the if statement is checking for, then execute the exact same logic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it reads the PrepareFns, are they the same thing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry. I meant L2118 here. for (LazyCallGraph::Node *N : Coroutines) {

With and without this block, the executed logic is identical. With this block, this PrepareFns gets replaced with replaceAllPrepares on L2113. Without this block, if the Coroutines collection happens to be empty, it goes to the L2118 loop without any iteration, it goes to L2146 to perform the same logic.

Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

@yuxuanchen1997
Copy link
Member Author

Sounds good.

Thanks.

@yuxuanchen1997 yuxuanchen1997 merged commit 0483f14 into llvm:main Jul 11, 2024
10 checks passed
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
…lvm#98392)

If `Coroutines.empty()` the following loop is going to be skipped
entirely and same goes for `PrepareFns.empty()`. These two conditions
here aren't useful and adds to complexity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants