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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/IR/BasicBlock.h:418
+  bool isInstrOrderValid() const {
+    return getSubclassDataFromValue() & 1;
+  }
----------------
mkazantsev wrote:
> 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.
I rewrote this to use a bitfield. I think it's easier to understand now.


================
Comment at: llvm/include/llvm/IR/BasicBlock.h:433
+  /// assert internally so that we get better location info.
+  bool validateInstrOrdering() const;
+
----------------
mkazantsev wrote:
> 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.
So, `isInstrOrderingValid` would conflict with `isInstrOrderValid`, which gives you the cached answer.

I'll just make it void and assert internally in !NDEBUG builds.


================
Comment at: llvm/unittests/IR/BasicBlockTest.cpp:186
+  Builder.SetInsertPoint(BB, I2->getIterator());
+  Instruction *I1a = Builder.CreateCall(Nop);
+  EXPECT_FALSE(BB->isInstrOrderValid());
----------------
mkazantsev wrote:
> Do you mind adding the similar check for `Instruction->removeFromParent` and `Instruction->eraseFromParent`? 
Done, but they don't invalidate ordering, so I check for that instead.


https://reviews.llvm.org/D51664