[PATCH] D23437: [LoopInfo] Add a routine for verification by recomputation.

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Sorry for the delay. Feel free to commit, this is all just nit picky stuff that can be sorted out and even changed afterward if desired.

In https://reviews.llvm.org/D23437#523726, @mzolotukhin wrote:

> I moved the new logic to `verify()` routine, does it look good? I'm not convinced that we need to recalculate `DT` though: we have a separate verifier for it and in my view recalculating it here would be functionality duplication to some extent. IOW I think we should rely on `DT` being correct and use verifiers to make sure it holds true; if we don't trust corresponding verifiers, we should fix them. Does it make sense?


It makes a certain amount of sense. I guess my view is more that if someone adds the loop verify pass because their loop pass is misbehaving, it'll be frustrating of the verify misses the issue because of an out-of-date dominator tree.

We could instead verify the dominator tree, or just compute a known-correct one. I could see reasonable arguments in both directions.

As perhaps a better rationale -- imagine someone is writing code to update LoopInfo (and possibly DominatorTree). They call verify as part of their update to debug stuff. Is it reasonable to have imposed an ordering requirement that the dominator tree update be applied first?

Anyways, this isn't a big deal, just that I somewhat prefer that where possible each verify step be as free-standing of other passes and state as possible in order to isolate problems. And here, we can compute the dominator tree with a single function call so it seems very much low-cost.

Some minor comments below about the particular code used. Happy for these to be "post commit" comments or whatever that you address as you go.


================
Comment at: include/llvm/Analysis/LoopInfoImpl.h:535-541
@@ +534,9 @@
+static void
+addInnerLoopsToHeadersMap(DenseMap<BlockT *, const LoopT *> &LoopHeaders,
+                          const LoopInfoBase<BlockT, LoopT> &LI,
+                          const LoopT &L) {
+  LoopHeaders[L.getHeader()] = &L;
+  for (LoopT *SL : L)
+    addInnerLoopsToHeadersMap(LoopHeaders, LI, *SL);
+}
+
----------------
Why not a lambda below? It wouldn't need to be a generic lambda as its already inside a function template and within that context you have specific types BlockT and LoopT already.

================
Comment at: include/llvm/Analysis/LoopInfoImpl.h:544
@@ +543,3 @@
+template <class BlockT, class LoopT>
+static bool compareLoops(const LoopT *L1, const LoopT *L2) {
+  BlockT *H1 = L1->getHeader();
----------------
Same question, why not a lambda?

================
Comment at: include/llvm/Analysis/LoopInfoImpl.h:549-552
@@ +548,6 @@
+    return false;
+  std::vector<BlockT *> BB1 = L1->getBlocks();
+  std::vector<BlockT *> BB2 = L2->getBlocks();
+  if (!compareVectors(BB1, BB2))
+    return false;
+
----------------
std::sort and std::equals? Seems simpler and there is no order concern.


https://reviews.llvm.org/D23437