CodeExtractor: NFC: Use Range based loop

[tejohnson]

llvm/lib/Transforms/Utils/CodeExtractor.cpp

963

You can use a range iterator over the users accessed from header directly, rather than creating a vector:

for (auto &U : header->users())

Looks like the code earlier at line 944-5 can be transformed similarly

[hiraditya]

Tried the following, I get test failures and crashes.

diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index 3e1bea77f6c..c4661fde0fb 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -929,8 +929,7 @@ Function *CodeExtractor::constructFunction(const ValueSet &inputs,
   // Rewrite branches to basic blocks outside of the loop to new dummy blocks
   // within the new function. This must be done before we lose track of which
   // blocks were originally in the code region.
-  std::vector<User *> Users(header->user_begin(), header->user_end());
-  for (auto &U : Users)
+  for (auto U : header->users())
     // The BasicBlock which contains the branch is not in the region
     // modify the branch target to a new block
     if (Instruction *I = dyn_cast<Instruction>(U))

Failing Tests (6):

LLVM :: Transforms/CodeExtractor/PartialInlineAndOr.ll
LLVM :: Transforms/CodeExtractor/PartialInlineOr.ll
LLVM :: Transforms/CodeExtractor/PartialInlineOrAnd.ll    // Crashed: Assertion `(Flags & RF_IgnoreMissingLocals) && "Referenced value not in value map!"' failed.
LLVM :: Transforms/HotColdSplit/eh-pads.ll
LLVM :: Transforms/HotColdSplit/outline-multiple-entry-region.ll
LLVM :: Transforms/HotColdSplit/unwind.ll

[fhahn]

In D68924#1707545, @hiraditya wrote:

Tried the following, I get test failures and crashes.

diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index 3e1bea77f6c..c4661fde0fb 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -929,8 +929,7 @@ Function *CodeExtractor::constructFunction(const ValueSet &inputs,
   // Rewrite branches to basic blocks outside of the loop to new dummy blocks
   // within the new function. This must be done before we lose track of which
   // blocks were originally in the code region.
-  std::vector<User *> Users(header->user_begin(), header->user_end());
-  for (auto &U : Users)
+  for (auto U : header->users())
     // The BasicBlock which contains the branch is not in the region
     // modify the branch target to a new block
     if (Instruction *I = dyn_cast<Instruction>(U))

Failing Tests (6):

LLVM :: Transforms/CodeExtractor/PartialInlineAndOr.ll
LLVM :: Transforms/CodeExtractor/PartialInlineOr.ll
LLVM :: Transforms/CodeExtractor/PartialInlineOrAnd.ll    // Crashed: Assertion `(Flags & RF_IgnoreMissingLocals) && "Referenced value not in value map!"' failed.
LLVM :: Transforms/HotColdSplit/eh-pads.ll
LLVM :: Transforms/HotColdSplit/outline-multiple-entry-region.ll
LLVM :: Transforms/HotColdSplit/unwind.ll

I guess you are seeing some segfaults, right? The problem is that in the loop, you change header's users (I->replaceUsesOfWith()), which invalidates the range iterator. I am not entirely sure how the users() iterator range is implemented, but you might be able to use make_early_inc_range(header->users()).

[hiraditya]

Good point, there's something about updating the users that copying maybe required. make_early_inc_range didn't work, I searched for uses cases of replaceUsesOfWith and I couldn't find any instance where we use 'users'.

[tejohnson]

LGTM

llvm/lib/Transforms/Utils/CodeExtractor.cpp

963

I missed the fact that the below loop is modifying the users list, so you can ignore my suggestion.

llvm/lib/Transforms/Utils/CodeExtractor.cpp

for (unsigned i = 0, e = inputs.size(); i != e; ++i, ++AI)
       AI->setName(inputs[i]->getName());
     for (unsigned i = 0, e = outputs.size(); i != e; ++i, ++AI)
       AI->setName(outputs[i]->getName()+".out");
   }
 
   // Rewrite branches to basic blocks outside of the loop to new dummy blocks
   // within the new function. This must be done before we lose track of which
   // blocks were originally in the code region.
   std::vector<User *> Users(header->user_begin(), header->user_end());
-  for (unsigned i = 0, e = Users.size(); i != e; ++i)
+  for (auto &U : Users)
     // The BasicBlock which contains the branch is not in the region
     // modify the branch target to a new block
-    if (Instruction *I = dyn_cast<Instruction>(Users[i]))
-      if (I->isTerminator() && !Blocks.count(I->getParent()) &&
-          I->getParent()->getParent() == oldFunction)
+    if (Instruction *I = dyn_cast<Instruction>(U))
+      if (I->isTerminator() && I->getFunction() == oldFunction &&
+          !Blocks.count(I->getParent()))
         I->replaceUsesOfWith(header, newHeader);
 
   return newFunction;
 }
 
 /// Erase lifetime.start markers which reference inputs to the extraction
 /// region, and insert the referenced memory into \p LifetimesStart.
 ///