[PATCH] D51664: [IR] Lazily number instructions for local dominance queries

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Any other thoughts on this?



================
Comment at: llvm/include/llvm/IR/BasicBlock.h:487
 
+#ifdef NDEBUG
+/// In release builds, this is a no-op. For !NDEBUG builds, the checks are
----------------
kuhar wrote:
> Does it make sense to disable this when EXPENSIVE_CHECKS are set?
I would assume EXPENSIVE_CHECKS implies !NDEBUG, and these checks are on when assertions enabled.


================
Comment at: llvm/lib/Analysis/OrderedInstructions.cpp:32
   if (InstA->getParent() == InstB->getParent())
     return localDominates(InstA, InstB);
   return DT->dominates(InstA->getParent(), InstB->getParent());
----------------
kuhar wrote:
> Is the separate function `localDominates` still needed? Seems like the body is trivial and could be inlined here?
Sure, fixed.


================
Comment at: llvm/lib/IR/BasicBlock.cpp:498
+  for (Instruction &I : *this)
+    I.Order = Order++;
+
----------------
kuhar wrote:
> Is it possible to use noncontiguous  indices? If the indices are spread apart, you should be able to perform most insertions without renumbering instructions.
I want to put that out of scope of the initial change. We can do all kinds of fancy tricks here to avoid invalidating the ordering, but it's hard to provide meaningfully better algorithmic guarantees. And, the more complex code will require more complex testing, and it might have bugs. I'd rather come back and implement a more complex algorithm once profiling shows that there is a bottleneck, especially since it's often easier to remove these bottlenecks by delaying insertion.


================
Comment at: llvm/lib/IR/SymbolTableListTraitsImpl.h:94
+  ItemParentClass *NewIP = getListOwner();
+  invalidateParentIListOrdering(NewIP);
+
----------------
kuhar wrote:
> Isn't it enough to invalidate only the indices of instructions that follow the first inserted one?
Yes, but recording that info and leveraging it is complex, and it doesn't change the asymptotic performance. We'd need more than a bit in BasicBlock to do it.


https://reviews.llvm.org/D51664