Page MenuHomePhabricator

CodeExtractor: NFC: Use Range based loop
ClosedPublic

Authored by hiraditya on Oct 12 2019, 6:51 PM.

Diff Detail

Event Timeline

hiraditya created this revision.Oct 12 2019, 6:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2019, 6:51 PM
tejohnson added inline comments.Oct 12 2019, 8:48 PM
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 added a comment.EditedOct 13 2019, 10:21 AM

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

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

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 accepted this revision.Oct 14 2019, 12:59 PM

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.

This revision is now accepted and ready to land.Oct 14 2019, 12:59 PM
This revision was automatically updated to reflect the committed changes.