[LoopInfo] Don't bail out early in getLoopID

[loladiro]

Summary

As far as I understand getLoopID needs to look at the intersection of

1. BasicBlocks in the loop and
2. Predecessors of the loop header

and check each of those terminators for a !loop metadata node. However,
what getLoopID is currently doing is iterate through the basic blocks
and require that every one of them have an edge back to the loop header
with the appropriate metadata. This seems incorrect and causes situations
in which a LoopID set with setLoopID cannot be subsequently retrieved with
getLoopID.

[fhahn]

Could you add a test for that? e.g in unittests/Analysis/LoopInfoTest.cpp

lib/Analysis/LoopInfo.cpp

224

Shouldn't getLoopLatches return exactly the nodes we are interested in (predecessors of the header in the loop) [1]? IIUC using it would simplify the code even further.

[1] https://llvm.org/doxygen/classllvm_1_1LoopBase.html#abbefed3df8ce43cf3f0c2f7fd004f6a4

[loladiro]

Updated to use getLoopLatches and add a test.

[fhahn]

Thanks for adding the test case! Just a few small comments left.

lib/Analysis/LoopInfo.cpp

221

Please reflow this comment

225

Usually we don't use this->

268

Usually we don't use this->.

269

No braces for one stmt blocks

unittests/Analysis/LoopInfoTest.cpp

108 ↗

(On Diff #146485)

not: capitialize first letter of variable names?

110 ↗

(On Diff #146485)

We can get all the info using LoopInfo I think. An assert that we have 2 latches should be enough?

133 ↗

(On Diff #146485)

Could you extend this test case to also check the cases where the latches contain differing ids and where one latch does not contain a loop id, while the other does? It should be possible to just change the attached loop ids I think.

[vtjnash]

@loladiro bump. It sounds like this was approved, but it doesn't apply cleanly now.

[loladiro]

Looks like this got fixed by @jdoerfert in rL341926 with a near identical patch and test. No rebase required.

[loladiro]

Actually, looks like the change to setLoopID is still relevant. I'll submit a new revision with just that change.

lib/Analysis/LoopInfo.cpp

 
 MDNode *Loop::getLoopID() const {
   MDNode *LoopID = nullptr;
   if (BasicBlock *Latch = getLoopLatch()) {
     LoopID = Latch->getTerminator()->getMetadata(LLVMContext::MD_loop);
   } else {
     assert(!getLoopLatch() &&
            "The loop should have no single latch at this point");
     // Go through each predecessor of the loop header and check the
     // terminator for the metadata.
     BasicBlock *H = getHeader();
     for (BasicBlock *BB : this->blocks()) {
       TerminatorInst *TI = BB->getTerminator();
-      MDNode *MD = nullptr;
 
       // Check if this terminator branches to the loop header.
-      for (BasicBlock *Successor : TI->successors()) {
-        if (Successor == H) {
-          MD = TI->getMetadata(LLVMContext::MD_loop);
-          break;
-        }
-      }
+      bool IsPredecessor = any_of(TI->successors(),
+        [=](BasicBlock *Successor) { return Successor == H; });
+      if (!IsPredecessor)
+        continue;
+
+      MDNode *MD = TI->getMetadata(LLVMContext::MD_loop);
       if (!MD)
         return nullptr;
 
       if (!LoopID)
         LoopID = MD;
       else if (MD != LoopID)
         return nullptr;
     }
assert(!getLoopLatch() &&
          "The loop should have no single latch at this point");
   BasicBlock *H = getHeader();
   for (BasicBlock *BB : this->blocks()) {
     TerminatorInst *TI = BB->getTerminator();
     for (BasicBlock *Successor : TI->successors()) {
       if (Successor == H)
         TI->setMetadata(LLVMContext::MD_loop, LoopID);
     }
   }
 }
 
 void Loop::setLoopAlreadyUnrolled() {
   MDNode *LoopID = getLoopID();
   // First remove any existing loop unrolling metadata.
   SmallVector<Metadata *, 4> MDs;
   // Reserve first location for self reference to the LoopID metadata node.
   MDs.push_back(nullptr);