[NFC][LoopIdiom] Make for loops more readable

[yurai007]

Summary

Patch simplifies for loops in LIR following LLVM guidelines: https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible.

[yurai007]

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp

667

That could be simplified a bit with 'seq' but extra headers need to included, so decided leave it as it is.

[craig.topper]

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp

667

I don't find this more readable.

I'm not sure what the L in LU was supposed to do. long is 32-bits on 64-bit Windows so if you were trying to make it the same size as size_t to match the return type of SL.size(), it didn't work.

[yurai007]

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp

667

I don't find this more readable.

Well, it's less verbose but indeed maybe not really more readable.

I'm not sure what the L in LU was supposed to do. long is 32-bits on 64-bit Windows so if you were trying to make it the same size as size_t to match the return type of SL.size(), it didn't work.

Yes, that was my intention but I failed miserably. I will go back to previous loop version without auto.

[nikic]

Please limit your use of auto to only the cases explicitly listed in https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable, i.e. if the type is already otherwise mentioned (cast/dyn_cast) or abstracted anyway (iterator typedefs). There is more nuance to this policy, but as you tend to heavily overuse auto I will recommend that you stick to these explicitly allowed cases only.

[yurai007]

Addressed all comments.

[nikic]

LGTM

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp

 /// with the specified backedge count.  This block is known to be in the current
 /// loop and not in any subloops.
 bool LoopIdiomRecognize::runOnLoopBlock(
     BasicBlock *BB, const SCEV *BECount,
     SmallVectorImpl<BasicBlock *> &ExitBlocks) {
   // We can only promote stores in this block if they are unconditionally
   // executed in the loop.  For a block to be unconditionally executed, it has
   // to dominate all the exit blocks of the loop.  Verify this now.
-  for (unsigned i = 0, e = ExitBlocks.size(); i != e; ++i)
-    if (!DT->dominates(BB, ExitBlocks[i]))
+  for (BasicBlock *ExitBlock : ExitBlocks)
+    if (!DT->dominates(BB, ExitBlock))
       return false;
 
   bool MadeChange = false;
   // Look for store instructions, which may be optimized to memset/memcpy.
   collectStores(BB);
 
   // Look for a single store or sets of stores with a common base, which can be
   // optimized into a memset (memset_pattern).  The latter most commonly happens
bool LoopIdiomRecognize::processLoopStores(SmallVectorImpl<StoreInst *> &SL,
                                            const SCEV *BECount, ForMemset For) {
   // Try to find consecutive stores that can be transformed into memsets.
   SetVector<StoreInst *> Heads, Tails;
   SmallDenseMap<StoreInst *, StoreInst *> ConsecutiveChain;
 
   // Do a quadratic search on all of the given stores and find
   // all of the pairs of stores that follow each other.
   SmallVector<unsigned, 16> IndexQueue;
   for (unsigned i = 0, e = SL.size(); i < e; ++i) {
     assert(SL[i]->isSimple() && "Expected only non-volatile stores.");
 
     Value *FirstStoredVal = SL[i]->getValueOperand();
     Value *FirstStorePtr = SL[i]->getPointerOperand();
     const SCEVAddRecExpr *FirstStoreEv =
         cast<SCEVAddRecExpr>(SE->getSCEV(FirstStorePtr));
     APInt FirstStride = getStoreStride(FirstStoreEv);
     unsigned FirstStoreSize = DL->getTypeStoreSize(SL[i]->getValueOperand()->getType());
bool LoopIdiomRecognize::processLoopStores(SmallVectorImpl<StoreInst *> &SL,
   }
 
   // We may run into multiple chains that merge into a single chain. We mark the
   // stores that we transformed so that we don't visit the same store twice.
   SmallPtrSet<Value *, 16> TransformedStores;
   bool Changed = false;
 
   // For stores that start but don't end a link in the chain:
-  for (SetVector<StoreInst *>::iterator it = Heads.begin(), e = Heads.end();
-       it != e; ++it) {
-    if (Tails.count(*it))
+  for (StoreInst *I : Heads) {
+    if (Tails.count(I))
       continue;
 
     // We found a store instr that starts a chain. Now follow the chain and try
     // to transform it.
     SmallPtrSet<Instruction *, 8> AdjacentStores;
-    StoreInst *I = *it;
-
     StoreInst *HeadStore = I;
     unsigned StoreSize = 0;
 
     // Collect the chain into a list.
     while (Tails.count(I) || Heads.count(I)) {
       if (TransformedStores.count(I))
         break;
       AdjacentStores.insert(I);
AccessSize = LocationSize::precise((BECst->getValue()->getZExtValue() + 1) *
                                        ConstSize->getValue()->getZExtValue());
 
   // TODO: For this to be really effective, we have to dive into the pointer
   // operand in the store.  Store to &A[i] of 100 will always return may alias
   // with store of &A[100], we need to StoreLoc to be "A" with size of 100,
   // which will then no-alias a store to &A[100].
   MemoryLocation StoreLoc(Ptr, AccessSize);
 
-  for (Loop::block_iterator BI = L->block_begin(), E = L->block_end(); BI != E;
-       ++BI)
-    for (Instruction &I : **BI)
+  for (BasicBlock *B : L->blocks())
+    for (Instruction &I : *B)
       if (IgnoredInsts.count(&I) == 0 &&
           isModOrRefSet(
               intersectModRef(AA.getModRefInfo(&I, StoreLoc), Access)))
         return true;
   return false;
 }
 
 // If we have a negative stride, Start refers to the end of the memory location
static bool detectPopcountIdiom(Loop *CurLoop, BasicBlock *PreCondBB,
   // step 3: Check the recurrence of variable X
   PhiX = getRecurrenceVar(VarX1, DefX2, LoopEntry);
   if (!PhiX)
     return false;
 
   // step 4: Find the instruction which count the population: cnt2 = cnt1 + 1
   {
     CountInst = nullptr;
-    for (BasicBlock::iterator Iter = LoopEntry->getFirstNonPHI()->getIterator(),
-                              IterE = LoopEntry->end();
-         Iter != IterE; Iter++) {
-      Instruction *Inst = &*Iter;
-      if (Inst->getOpcode() != Instruction::Add)
+    for (Instruction &Inst : llvm::make_range(
+             LoopEntry->getFirstNonPHI()->getIterator(), LoopEntry->end())) {
+      if (Inst.getOpcode() != Instruction::Add)
         continue;
 
-      ConstantInt *Inc = dyn_cast<ConstantInt>(Inst->getOperand(1));
+      ConstantInt *Inc = dyn_cast<ConstantInt>(Inst.getOperand(1));
       if (!Inc || !Inc->isOne())
         continue;
 
-      PHINode *Phi = getRecurrenceVar(Inst->getOperand(0), Inst, LoopEntry);
+      PHINode *Phi = getRecurrenceVar(Inst.getOperand(0), &Inst, LoopEntry);
       if (!Phi)
         continue;
 
       // Check if the result of the instruction is live of the loop.
       bool LiveOutLoop = false;
-      for (User *U : Inst->users()) {
+      for (User *U : Inst.users()) {
         if ((cast<Instruction>(U))->getParent() != LoopEntry) {
           LiveOutLoop = true;
           break;
         }
       }
 
       if (LiveOutLoop) {
-        CountInst = Inst;
+        CountInst = &Inst;
         CountPhi = Phi;
         break;
       }
     }
 
     if (!CountInst)
       return false;
   }
static bool detectShiftUntilZeroIdiom(Loop *CurLoop, const DataLayout &DL,
 
   // step 4: Find the instruction which count the CTLZ: cnt.next = cnt + 1
   //         or cnt.next = cnt + -1.
   // TODO: We can skip the step. If loop trip count is known (CTLZ),
   //       then all uses of "cnt.next" could be optimized to the trip count
   //       plus "cnt0". Currently it is not optimized.
   //       This step could be used to detect POPCNT instruction:
   //       cnt.next = cnt + (x.next & 1)
-  for (BasicBlock::iterator Iter = LoopEntry->getFirstNonPHI()->getIterator(),
-                            IterE = LoopEntry->end();
-       Iter != IterE; Iter++) {
-    Instruction *Inst = &*Iter;
-    if (Inst->getOpcode() != Instruction::Add)
+  for (Instruction &Inst : llvm::make_range(
+           LoopEntry->getFirstNonPHI()->getIterator(), LoopEntry->end())) {
+    if (Inst.getOpcode() != Instruction::Add)
       continue;
 
-    ConstantInt *Inc = dyn_cast<ConstantInt>(Inst->getOperand(1));
+    ConstantInt *Inc = dyn_cast<ConstantInt>(Inst.getOperand(1));
     if (!Inc || (!Inc->isOne() && !Inc->isMinusOne()))
       continue;
 
-    PHINode *Phi = getRecurrenceVar(Inst->getOperand(0), Inst, LoopEntry);
+    PHINode *Phi = getRecurrenceVar(Inst.getOperand(0), &Inst, LoopEntry);
     if (!Phi)
       continue;
 
-    CntInst = Inst;
+    CntInst = &Inst;
     CntPhi = Phi;
     break;
   }
   if (!CntInst)
     return false;
 
   return true;
 }