[PATCH] D46460: [LoopInfo] Don't bail out early in getLoopID

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

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



================
Comment at: lib/Analysis/LoopInfo.cpp:221
            "The loop should have no single latch at this point");
-    // Go through each predecessor of the loop header and check the
+    // Go through each loop latch and check the
     // terminator for the metadata.
----------------
Please reflow this comment


================
Comment at: lib/Analysis/LoopInfo.cpp:224
+    SmallVector<BasicBlock *, 4> LoopLatches;
+    this->getLoopLatches(LoopLatches);
+    for (BasicBlock *BB : LoopLatches) {
----------------
Usually we don't use `this->`


================
Comment at: lib/Analysis/LoopInfo.cpp:256
+  SmallVector<BasicBlock *, 4> LoopLatches;
+  this->getLoopLatches(LoopLatches);
+  for (BasicBlock *BB : LoopLatches) {
----------------
Usually we don't use `this->`.


================
Comment at: lib/Analysis/LoopInfo.cpp:257
+  this->getLoopLatches(LoopLatches);
+  for (BasicBlock *BB : LoopLatches) {
+    BB->getTerminator()->setMetadata(LLVMContext::MD_loop, LoopID);
----------------
No braces for one stmt blocks


================
Comment at: unittests/Analysis/LoopInfoTest.cpp:108
+  runWithLoopInfo(*M, "foo", [&](Function &F, LoopInfo &LI) {
+    BasicBlock *header = nullptr, *latch1 = nullptr, *latch2 = nullptr;
+    // Iterate over the function and find our interersting blocks
----------------
not: capitialize first letter of variable names?


================
Comment at: unittests/Analysis/LoopInfoTest.cpp:110
+    // Iterate over the function and find our interersting blocks
+    for (BasicBlock &BB : F) {
+      if (BB.getName() == "header")
----------------
We can get all the info using LoopInfo I think. An assert that we have 2 latches should be enough?


================
Comment at: unittests/Analysis/LoopInfoTest.cpp:133
+              LoopID);
+    EXPECT_EQ(L->getLoopID(), LoopID);
+  });
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D46460