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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

I don't see any fundamental flaws in the algorithm, it looks pretty robust. I have some nit comments, otherwise it LGTM. (Note that I'm maybe not the most qualified person to approve changes in such fundamental components as BasicBlock and Instruction, but this change seems profitable).



================
Comment at: llvm/include/llvm/IR/BasicBlock.h:418
+  bool isInstrOrderValid() const {
+    return getSubclassDataFromValue() & 1;
+  }
----------------
I would rather use some named constant instead of `1`; it is widespread use across the code and may be confusing for a reader. Just a suggestion.


================
Comment at: llvm/include/llvm/IR/BasicBlock.h:433
+  /// assert internally so that we get better location info.
+  bool validateInstrOrdering() const;
+
----------------
george.burgess.iv wrote:
> george.burgess.iv wrote:
> > nit: I'd expect a function named this to `assert()`. Can we rename it to something that sounds more boolean-y like `isInstrOrderingValid()` (?) and/or add `LLVM_NODISCARD`?
> nit * 2: I don't know what precedent is set elsewhere, but personally, I feel that if the intent is for this to only be used in NDEBUG builds, we should wrap this function decl in `#ifndef NDEBUG`.
> 
> Mostly because I'd prefer the compiler tells me to reconsider my life choices over silently getting potentially-incorrect results. If it turns out that there's a case where the latter is preferable, it's a ~4 line diff. :)
Agreed here.


================
Comment at: llvm/lib/IR/Instruction.cpp:102
+bool Instruction::comesBefore(const Instruction *Other) const {
+  assert(Parent == Other->Parent && "cross-BB instruction order comparison");
+  if (!Parent->isInstrOrderValid())
----------------
Maybe also makes sense to assert that Parent is not nullptr (i.e. instructions not detached).


================
Comment at: llvm/unittests/IR/BasicBlockTest.cpp:152
+  EXPECT_FALSE(Add->comesBefore(Add));
+  BB.invalidateOrders();
+  EXPECT_TRUE(Add->comesBefore(Ret));
----------------
`EXPECT_TRUE/FALSE(BB->isInstrOrderValid())` before and after that to make sure that it works at all?


================
Comment at: llvm/unittests/IR/BasicBlockTest.cpp:186
+  Builder.SetInsertPoint(BB, I2->getIterator());
+  Instruction *I1a = Builder.CreateCall(Nop);
+  EXPECT_FALSE(BB->isInstrOrderValid());
----------------
Do you mind adding the similar check for `Instruction->removeFromParent` and `Instruction->eraseFromParent`? 


https://reviews.llvm.org/D51664