[NFC][Coroutines] Remove integer indexing in several CoroSplit loops #115954

TylerNowicki · 2024-11-12T22:53:53Z

Use helpers such as llvm::enumerate and llvm::zip in places to avoid using loop counters. Also refactored AnyRetconABI::splitCoroutine to avoid some awkward indexing that came about by putting ContinuationPhi into the ReturnPHIs vector and mistaking i with I and e with E.

llvmbot · 2024-11-12T22:54:27Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-coroutines

Author: Tyler Nowicki (TylerNowicki)

Changes

Use helpers such as llvm::enumerate and llvm::zip in places to avoid using loop counters. Also refactored AnyRetconABI::splitCoroutine to avoid some awkward indexing that came about by putting ContinuationPhi into the ReturnPHIs vector and mistaking i with I and e with E.

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

1 Files Affected:

(modified) llvm/lib/Transforms/Coroutines/CoroSplit.cpp (+38-38)

diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index 25a962ddf1b0da..b4121013187ffd 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -22,6 +22,7 @@
 #include "CoroInternal.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/PriorityWorklist.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
@@ -628,8 +629,8 @@ void CoroCloner::replaceRetconOrAsyncSuspendUses() {
 
   // Otherwise, we need to create an aggregate.
   Value *Agg = PoisonValue::get(NewS->getType());
-  for (size_t I = 0, E = Args.size(); I != E; ++I)
-    Agg = Builder.CreateInsertValue(Agg, Args[I], I);
+  for (auto Arg : llvm::enumerate(Args))
+    Agg = Builder.CreateInsertValue(Agg, Arg.value(), Arg.index());
 
   NewS->replaceAllUsesWith(Agg);
 }
@@ -1833,8 +1834,8 @@ void coro::AsyncABI::splitCoroutine(Function &F, coro::Shape &Shape,
 
   // Create a continuation function for each of the suspend points.
   Clones.reserve(Shape.CoroSuspends.size());
-  for (size_t Idx = 0, End = Shape.CoroSuspends.size(); Idx != End; ++Idx) {
-    auto *Suspend = cast<CoroSuspendAsyncInst>(Shape.CoroSuspends[Idx]);
+  for (auto CS : llvm::enumerate(Shape.CoroSuspends)) {
+    auto *Suspend = cast<CoroSuspendAsyncInst>(CS.value());
 
     // Create the clone declaration.
     auto ResumeNameSuffix = ".resume.";
@@ -1850,8 +1851,8 @@ void coro::AsyncABI::splitCoroutine(Function &F, coro::Shape &Shape,
     }
     auto *Continuation = createCloneDeclaration(
         F, Shape,
-        UseSwiftMangling ? ResumeNameSuffix + Twine(Idx) + "_"
-                         : ResumeNameSuffix + Twine(Idx),
+        UseSwiftMangling ? ResumeNameSuffix + Twine(CS.index()) + "_"
+                         : ResumeNameSuffix + Twine(CS.index()),
         NextF, Suspend);
     Clones.push_back(Continuation);
 
@@ -1884,11 +1885,11 @@ void coro::AsyncABI::splitCoroutine(Function &F, coro::Shape &Shape,
   }
 
   assert(Clones.size() == Shape.CoroSuspends.size());
-  for (size_t Idx = 0, End = Shape.CoroSuspends.size(); Idx != End; ++Idx) {
-    auto *Suspend = Shape.CoroSuspends[Idx];
-    auto *Clone = Clones[Idx];
+  for (auto CS : llvm::enumerate(Shape.CoroSuspends)) {
+    auto *Suspend = CS.value();
+    auto *Clone = Clones[CS.index()];
 
-    CoroCloner::createClone(F, "resume." + Twine(Idx), Shape, Clone, Suspend,
+    CoroCloner::createClone(F, "resume." + Twine(CS.index()), Shape, Clone, Suspend,
                             TTI);
   }
 }
@@ -1938,6 +1939,7 @@ void coro::AnyRetconABI::splitCoroutine(Function &F, coro::Shape &Shape,
 
   // Create a unique return block.
   BasicBlock *ReturnBB = nullptr;
+  PHINode *ContinuationPhi = nullptr;
   SmallVector<PHINode *, 4> ReturnPHIs;
 
   // Create all the functions in order after the main function.
@@ -1945,12 +1947,12 @@ void coro::AnyRetconABI::splitCoroutine(Function &F, coro::Shape &Shape,
 
   // Create a continuation function for each of the suspend points.
   Clones.reserve(Shape.CoroSuspends.size());
-  for (size_t i = 0, e = Shape.CoroSuspends.size(); i != e; ++i) {
-    auto Suspend = cast<CoroSuspendRetconInst>(Shape.CoroSuspends[i]);
+  for (auto CS : llvm::enumerate(Shape.CoroSuspends)) {
+    auto Suspend = cast<CoroSuspendRetconInst>(CS.value());
 
     // Create the clone declaration.
     auto Continuation =
-        createCloneDeclaration(F, Shape, ".resume." + Twine(i), NextF, nullptr);
+        createCloneDeclaration(F, Shape, ".resume." + Twine(CS.index()), NextF, nullptr);
     Clones.push_back(Continuation);
 
     // Insert a branch to the unified return block immediately before
@@ -1968,17 +1970,16 @@ void coro::AnyRetconABI::splitCoroutine(Function &F, coro::Shape &Shape,
 
       IRBuilder<> Builder(ReturnBB);
 
-      // Create PHIs for all the return values.
-      assert(ReturnPHIs.empty());
-
       // First, the continuation.
-      ReturnPHIs.push_back(Builder.CreatePHI(Continuation->getType(),
-                                             Shape.CoroSuspends.size()));
+      ContinuationPhi = Builder.CreatePHI(Continuation->getType(),
+                                             Shape.CoroSuspends.size());
+
+      // Create PHIs for all other return values.
+      assert(ReturnPHIs.empty());
 
       // Next, all the directly-yielded values.
       for (auto *ResultTy : Shape.getRetconResultTypes())
-        ReturnPHIs.push_back(
-            Builder.CreatePHI(ResultTy, Shape.CoroSuspends.size()));
+        ReturnPHIs.push_back(Builder.CreatePHI(ResultTy, Shape.CoroSuspends.size()));
 
       // Build the return value.
       auto RetTy = F.getReturnType();
@@ -1987,18 +1988,18 @@ void coro::AnyRetconABI::splitCoroutine(Function &F, coro::Shape &Shape,
       // We can't rely on the types matching up because that type would
       // have to be infinite.
       auto CastedContinuationTy =
-          (ReturnPHIs.size() == 1 ? RetTy : RetTy->getStructElementType(0));
+          (ReturnPHIs.empty() ? RetTy : RetTy->getStructElementType(0));
       auto *CastedContinuation =
-          Builder.CreateBitCast(ReturnPHIs[0], CastedContinuationTy);
+          Builder.CreateBitCast(ContinuationPhi, CastedContinuationTy);
 
-      Value *RetV;
-      if (ReturnPHIs.size() == 1) {
-        RetV = CastedContinuation;
-      } else {
+      Value *RetV = CastedContinuation;
+      if (!ReturnPHIs.empty()) {
+        auto ValueIdx = 0;
         RetV = PoisonValue::get(RetTy);
-        RetV = Builder.CreateInsertValue(RetV, CastedContinuation, 0);
-        for (size_t I = 1, E = ReturnPHIs.size(); I != E; ++I)
-          RetV = Builder.CreateInsertValue(RetV, ReturnPHIs[I], I);
+        RetV = Builder.CreateInsertValue(RetV, CastedContinuation, ValueIdx++);
+
+        for (auto Phi : ReturnPHIs)
+          RetV = Builder.CreateInsertValue(RetV, Phi, ValueIdx++);
       }
 
       Builder.CreateRet(RetV);
@@ -2006,19 +2007,18 @@ void coro::AnyRetconABI::splitCoroutine(Function &F, coro::Shape &Shape,
 
     // Branch to the return block.
     Branch->setSuccessor(0, ReturnBB);
-    ReturnPHIs[0]->addIncoming(Continuation, SuspendBB);
-    size_t NextPHIIndex = 1;
-    for (auto &VUse : Suspend->value_operands())
-      ReturnPHIs[NextPHIIndex++]->addIncoming(&*VUse, SuspendBB);
-    assert(NextPHIIndex == ReturnPHIs.size());
+    assert(ContinuationPhi);
+    ContinuationPhi->addIncoming(Continuation, SuspendBB);
+    for (auto [Phi, VUse] : llvm::zip_equal(ReturnPHIs, Suspend->value_operands()))
+      Phi->addIncoming(VUse, SuspendBB);
   }
 
   assert(Clones.size() == Shape.CoroSuspends.size());
-  for (size_t i = 0, e = Shape.CoroSuspends.size(); i != e; ++i) {
-    auto Suspend = Shape.CoroSuspends[i];
-    auto Clone = Clones[i];
+  for (auto CS : llvm::enumerate(Shape.CoroSuspends)) {
+    auto Suspend = CS.value();
+    auto Clone = Clones[CS.index()];
 
-    CoroCloner::createClone(F, "resume." + Twine(i), Shape, Clone, Suspend,
+    CoroCloner::createClone(F, "resume." + Twine(CS.index()), Shape, Clone, Suspend,
                             TTI);
   }
 }

github-actions · 2024-11-12T22:57:17Z

✅ With the latest revision this PR passed the C/C++ code formatter.

yuxuanchen1997 · 2024-11-12T23:12:40Z

For llvm::enumerate I think you can also have structured binding instead of CS.value() or CS.index() calls.

for (const auto &[Index, Value] : llvm::enumerate(Vec)) { ... }

TylerNowicki · 2024-11-14T22:51:23Z

For llvm::enumerate I think you can also have structured binding instead of CS.value() or CS.index() calls.
for (const auto &[Index, Value] : llvm::enumerate(Vec)) { ... }

Thanks! I will try this again and post another PR. I briefly tried it in one place, but it caused some build errors:

> for (auto [Arg, Index] : llvm::enumerate(Args))
llvm-project/llvm/lib/Transforms/Coroutines/CoroSplit.cpp:633:42: error: cannot initialize a parameter of type 'llvm::Value *' with an lvalue of type 'std::tuple_element<0, std::tuple<unsigned long, llvm::Value *&>>::type' (aka 'unsigned long')
Agg = Builder.CreateInsertValue(Agg, Arg, Index);
^~~
/home/tyler/llvm-project/llvm/include/llvm/IR/IRBuilder.h:2549:47: note: passing argument to parameter 'Val' here
Value *CreateInsertValue(Value *Agg, Value *Val, ArrayRef Idxs,

> for (auto &[Arg, Index] : llvm::enumerate(Args))
/home/tyler/llvm-project/llvm/lib/Transforms/Coroutines/CoroSplit.cpp:632:14: error: non-const lvalue reference to type 'enumerator_result<...>' cannot bind to a temporary of type 'enumerator_result<...>'
for (auto &[Arg, Index] : llvm::enumerate(Args))
^ ~
/home/tyler/llvm-project/llvm/include/llvm/ADT/STLExtras.h:827:12: note: selected 'begin' function with iterator type 'llvm::detail::zippy<llvm::detail::zip_enumerator, llvm::detail::index_stream, llvm::SmallVector<llvm::Value *, 8> &>::iterator' (aka 'zip_enumerator<llvm::detail::index_iterator, llvm::Value **>')
iterator begin() { return begin_impl(IndexSequence{}); }

yuxuanchen1997 · 2024-11-15T00:27:37Z

For llvm::enumerate I think you can also have structured binding instead of CS.value() or CS.index() calls.
for (const auto &[Index, Value] : llvm::enumerate(Vec)) { ... }
Thanks! I will try this again and post another PR. I briefly tried it in one place, but it caused some build errors:

> for (auto [Arg, Index] : llvm::enumerate(Args)) llvm-project/llvm/lib/Transforms/Coroutines/CoroSplit.cpp:633:42: error: cannot initialize a parameter of type 'llvm::Value *' with an lvalue of type 'std::tuple_element<0, std::tuple<unsigned long, llvm::Value *&>>::type' (aka 'unsigned long') Agg = Builder.CreateInsertValue(Agg, Arg, Index); ^~~ /home/tyler/llvm-project/llvm/include/llvm/IR/IRBuilder.h:2549:47: note: passing argument to parameter 'Val' here Value *CreateInsertValue(Value *Agg, Value *Val, ArrayRef Idxs,

> for (auto &[Arg, Index] : llvm::enumerate(Args)) /home/tyler/llvm-project/llvm/lib/Transforms/Coroutines/CoroSplit.cpp:632:14: error: non-const lvalue reference to type 'enumerator_result<...>' cannot bind to a temporary of type 'enumerator_result<...>' for (auto &[Arg, Index] : llvm::enumerate(Args)) ^ ~ /home/tyler/llvm-project/llvm/include/llvm/ADT/STLExtras.h:827:12: note: selected 'begin' function with iterator type 'llvm::detail::zippy<llvm::detail::zip_enumerator, llvm::detail::index_stream, llvm::SmallVector<llvm::Value *, 8> &>::iterator' (aka 'zip_enumerator<llvm::detail::index_iterator, llvm::Value **>') iterator begin() { return begin_impl(IndexSequence{}); }

This just means you have Arg and Index in incorrect order, right?

TylerNowicki · 2024-11-15T14:51:31Z

Thanks! I need a face palm reaction emoji...

[Coroutines] Remove integer indexing in several loops

83773b1

TylerNowicki requested a review from ChuanqiXu9 November 12, 2024 22:53

TylerNowicki self-assigned this Nov 12, 2024

llvmbot added coroutines C++20 coroutines llvm:transforms labels Nov 12, 2024

[Coroutines] Clang format

500f9cd

ChuanqiXu9 approved these changes Nov 13, 2024

View reviewed changes

TylerNowicki merged commit ba623e1 into llvm:main Nov 14, 2024
8 checks passed

TylerNowicki deleted the amd/dev/tnowicki/replace.int.loop.counters branch November 15, 2024 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NFC][Coroutines] Remove integer indexing in several CoroSplit loops #115954

[NFC][Coroutines] Remove integer indexing in several CoroSplit loops #115954

Uh oh!

TylerNowicki commented Nov 12, 2024

Uh oh!

llvmbot commented Nov 12, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Nov 12, 2024 •

edited

Loading

Uh oh!

yuxuanchen1997 commented Nov 12, 2024

Uh oh!

TylerNowicki commented Nov 14, 2024

Uh oh!

Uh oh!

yuxuanchen1997 commented Nov 15, 2024

Uh oh!

TylerNowicki commented Nov 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[NFC][Coroutines] Remove integer indexing in several CoroSplit loops #115954

[NFC][Coroutines] Remove integer indexing in several CoroSplit loops #115954

Uh oh!

Conversation

TylerNowicki commented Nov 12, 2024

Uh oh!

llvmbot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuxuanchen1997 commented Nov 12, 2024

Uh oh!

TylerNowicki commented Nov 14, 2024

Uh oh!

Uh oh!

yuxuanchen1997 commented Nov 15, 2024

Uh oh!

TylerNowicki commented Nov 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

llvmbot commented Nov 12, 2024 •

edited

Loading

github-actions bot commented Nov 12, 2024 •

edited

Loading