[mem2reg] Use range loops (NFCI)

[modocache]

Summary

Several for loops in PromoteMemoryToRegister.cpp leave their increment
expression empty, instead incrementing the iterator within the for loop
body. I believe this is because these loops were previously implemented
as while loops; see https://reviews.llvm.org/rL188327.

Incrementing the iterator within the body of the for loop instead of
in its increment expression makes it seem like the iterator will be
modified or conditionally incremented within the loop, but that is not
the case in these loops.

Instead, use range loops.

Test Plan: check-llvm

[bkramer]

Looks like all of these could be written as for-range loops for (User *U : Foo->users()) now.

[davide]

yes, please use Range loops here. Otherwise the idea is fine.

[modocache]

Thanks for the review! I've updated the diff to use range loops instead.

[davide]

lgtm, thanks.

[bkramer]

lg

[modocache]

Great, thank you both for the reviews!

[bjope]

llvm/trunk/lib/Transforms/Utils/PromoteMemoryToRegister.cpp

329 ↗

(On Diff #134966)

I think the old code was more safe since it does not depend as much on how the iterators are implemented. The instruction "I" is actually deleted inside the loop. So stepping the iterator after deleting "I" could be dangerous if stepping of the iterator involves dereferencing of the object that the iterator refers. Or am I missing something?
I would at least assume that the old code was written the way it was just because the datastructure that we iterate over can change during iteration. So it should be more safe to step the iterator to the "next" element first, and then remove the "current" element.

[modocache]

llvm/trunk/lib/Transforms/Utils/PromoteMemoryToRegister.cpp

329 ↗

(On Diff #134966)

Aha, that's a very good point, thanks! I was looking at the references to U but didn't see that I was being erased. I guess the same can be said for the loop below as well. So of the three loops in this diff, only the first loop appears to be convertible to a range loop.

lib/Transforms/Utils/PromoteMemoryToRegister.cpp

struct AllocaInfo {
   /// Scan the uses of the specified alloca, filling in the AllocaInfo used
   /// by the rest of the pass to reason about the uses of this alloca.
   void AnalyzeAlloca(AllocaInst *AI) {
     clear();
 
     // As we scan the uses of the alloca instruction, keep track of stores,
     // and decide whether all of the loads and stores to the alloca are within
     // the same basic block.
-    for (auto UI = AI->user_begin(), E = AI->user_end(); UI != E;) {
-      Instruction *User = cast<Instruction>(*UI++);
+    for (User *U : AI->users()) {
+      Instruction *User = cast<Instruction>(U);
 
       if (StoreInst *SI = dyn_cast<StoreInst>(User)) {
         // Remember the basic blocks which define new values for the alloca
         DefiningBlocks.push_back(SI->getParent());
         AllocaPointerVal = SI->getOperand(0);
         OnlyStore = SI;
       } else {
         LoadInst *LI = cast<LoadInst>(User);
static void addAssumeNonNull(AssumptionCache *AC, LoadInst *LI) {
   CI->insertAfter(LoadNotNull);
   AC->registerAssumption(CI);
 }
 
 static void removeLifetimeIntrinsicUsers(AllocaInst *AI) {
   // Knowing that this alloca is promotable, we know that it's safe to kill all
   // instructions except for load and store.
 
-  for (auto UI = AI->user_begin(), UE = AI->user_end(); UI != UE;) {
-    Instruction *I = cast<Instruction>(*UI);
-    ++UI;
+  for (User *U : AI->users()) {
+    Instruction *I = cast<Instruction>(U);
     if (isa<LoadInst>(I) || isa<StoreInst>(I))
       continue;
 
     if (!I->getType()->isVoidTy()) {
       // The only users of this bitcast/GEP instruction are lifetime intrinsics.
       // Follow the use/def chain to erase them now instead of leaving it for
       // dead code elimination later.
       for (auto UUI = I->user_begin(), UUE = I->user_end(); UUI != UUE;) {
static bool rewriteSingleStoreAlloca(AllocaInst *AI, AllocaInfo &Info,
   StoreInst *OnlyStore = Info.OnlyStore;
   bool StoringGlobalVal = !isa<Instruction>(OnlyStore->getOperand(0));
   BasicBlock *StoreBB = OnlyStore->getParent();
   int StoreIndex = -1;
 
   // Clear out UsingBlocks.  We will reconstruct it here if needed.
   Info.UsingBlocks.clear();
 
-  for (auto UI = AI->user_begin(), E = AI->user_end(); UI != E;) {
-    Instruction *UserInst = cast<Instruction>(*UI++);
+  for (User *U : AI->users()) {
+    Instruction *UserInst = cast<Instruction>(U);
     if (!isa<LoadInst>(UserInst)) {
       assert(UserInst == OnlyStore && "Should only have load/stores");
       continue;
     }
     LoadInst *LI = cast<LoadInst>(UserInst);
 
     // Okay, if we have a load from the alloca, we want to replace it with the
     // only value stored to the alloca.  We can do this if the value is
if (StoreInst *SI = dyn_cast<StoreInst>(U))
       StoresByIndex.push_back(std::make_pair(LBI.getInstructionIndex(SI), SI));
 
   // Sort the stores by their index, making it efficient to do a lookup with a
   // binary search.
   std::sort(StoresByIndex.begin(), StoresByIndex.end(), less_first());
 
   // Walk all of the loads from this alloca, replacing them with the nearest
   // store above them, if any.
-  for (auto UI = AI->user_begin(), E = AI->user_end(); UI != E;) {
-    LoadInst *LI = dyn_cast<LoadInst>(*UI++);
+  for (User *U : AI->users()) {
+    LoadInst *LI = dyn_cast<LoadInst>(U);
     if (!LI)
       continue;
 
     unsigned LoadIdx = LBI.getInstructionIndex(LI);
 
     // Find the nearest store that has a lower index than this load.
     StoresByIndexTy::iterator I =
         std::lower_bound(StoresByIndex.begin(), StoresByIndex.end(),