[PATCH] D51523: Return "[NFC] Add severe validation of InstructionPrecedenceTracking"

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

LGTM w/ changes made before submit.



================
Comment at: include/llvm/Analysis/InstructionPrecedenceTracking.h:42
+  /// Asserts that the cached info for \p BB is up-to-date. The behavior of a
+  /// query to a block with invalid cache is undefined, so we should avoid this
+  /// situation.
----------------
You're better off defining the behaviour as being erroneous.  (i.e. illegal).  Suggest alternate wording:
This helps to catch the usage error of accessing a block without properly invalidating after a previous transform.  


================
Comment at: lib/Analysis/InstructionPrecedenceTracking.cpp:77
+#ifndef NDEBUG
+void InstructionPrecedenceTracking::validate(const BasicBlock *BB) const {
+  auto It = FirstSpecialInsts.find(BB);
----------------
Optional, can be follow up: You'd be better to reduce the scope of the conditional compilation by using a conditional early return in this function.  


================
Comment at: lib/Analysis/InstructionPrecedenceTracking.cpp:101
+
+  // Check that all blocks with cached values are marked as known.
+  for (auto &It : FirstSpecialInsts)
----------------
You can write this as:
assert(KnownBlocks.size() == FirstSpecialInsts.size())

since you've already validated each bloc, if there are no other special instructions, no further validation is needed.


https://reviews.llvm.org/D51523