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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/IR/BasicBlock.cpp:498
+  for (Instruction &I : *this)
+    I.Order = Order++;
+
----------------
rnk wrote:
> 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.
Sure, makes perfect sense. I'm not very familiar with the IR part of llvm, but I'd prefer to see a comment that explains that in a relevant place if you believe that this is a good future direction.


================
Comment at: llvm/lib/IR/SymbolTableListTraitsImpl.h:94
+  ItemParentClass *NewIP = getListOwner();
+  invalidateParentIListOrdering(NewIP);
+
----------------
rnk wrote:
> 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.
Makes sense.
How expensive is it to add new data members to BasicBlock? Do you know of any attempts to stick some data inside and measure how it affects compilation times? 


https://reviews.llvm.org/D51664