[llvm] r341526 - Return "[NFC] Add severe validation of InstructionPrecedenceTracking"

[Max Kazantsev via llvm-commits]

Author: mkazantsev
Date: Thu Sep  6 01:33:02 2018
New Revision: 341526

URL: http://llvm.org/viewvc/llvm-project?rev=341526&view=rev
Log:
Return "[NFC] Add severe validation of InstructionPrecedenceTracking"

This validation patch has been reverted as rL341147 because of conserns raised by
@reames. This revision returns it as is to raise a discussion and address the concerns.

Differential Revision: https://reviews.llvm.org/D51523
Reviewed By: reames

Modified:
    llvm/trunk/include/llvm/Analysis/InstructionPrecedenceTracking.h
    llvm/trunk/lib/Analysis/InstructionPrecedenceTracking.cpp

Modified: llvm/trunk/include/llvm/Analysis/InstructionPrecedenceTracking.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/InstructionPrecedenceTracking.h?rev=341526&r1=341525&r2=341526&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Analysis/InstructionPrecedenceTracking.h (original)
+++ llvm/trunk/include/llvm/Analysis/InstructionPrecedenceTracking.h Thu Sep  6 01:33:02 2018
@@ -37,6 +37,18 @@ class InstructionPrecedenceTracking {
   // Fills information about the given block's special instructions.
   void fill(const BasicBlock *BB);
 
+#ifndef NDEBUG
+  /// Asserts that the cached info for \p BB is up-to-date. This helps to catch
+  /// the usage error of accessing a block without properly invalidating after a
+  /// previous transform.
+  void validate(const BasicBlock *BB) const;
+
+  /// Asserts whether or not the contents of this tracking is up-to-date. This
+  /// helps to catch the usage error of accessing a block without properly
+  /// invalidating after a previous transform.
+  void validateAll() const;
+#endif
+
 protected:
   InstructionPrecedenceTracking(DominatorTree *DT)
       : OI(OrderedInstructions(DT)) {}

Modified: llvm/trunk/lib/Analysis/InstructionPrecedenceTracking.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionPrecedenceTracking.cpp?rev=341526&r1=341525&r2=341526&view=diff
==============================================================================
--- llvm/trunk/lib/Analysis/InstructionPrecedenceTracking.cpp (original)
+++ llvm/trunk/lib/Analysis/InstructionPrecedenceTracking.cpp Thu Sep  6 01:33:02 2018
@@ -23,8 +23,25 @@
 
 using namespace llvm;
 
+#ifndef NDEBUG
+static cl::opt<bool> ExpensiveAsserts(
+    "ipt-expensive-asserts",
+    cl::desc("Perform expensive assert validation on every query to Instruction"
+             " Precedence Tracking"),
+    cl::init(false), cl::Hidden);
+#endif
+
 const Instruction *InstructionPrecedenceTracking::getFirstSpecialInstruction(
     const BasicBlock *BB) {
+#ifndef NDEBUG
+  // If there is a bug connected to invalid cache, turn on ExpensiveAsserts to
+  // catch this situation as early as possible.
+  if (ExpensiveAsserts)
+    validateAll();
+  else
+    validate(BB);
+#endif
+
   if (!KnownBlocks.count(BB))
     fill(BB);
   auto *FirstICF = FirstSpecialInsts.lookup(BB);
@@ -56,6 +73,45 @@ void InstructionPrecedenceTracking::fill
   KnownBlocks.insert(BB);
 }
 
+#ifndef NDEBUG
+void InstructionPrecedenceTracking::validate(const BasicBlock *BB) const {
+  // If we don't know anything about this block, make sure we don't store
+  // a bucket for it in FirstSpecialInsts map.
+  if (!KnownBlocks.count(BB)) {
+    assert(FirstSpecialInsts.find(BB) == FirstSpecialInsts.end() && "Must be!");
+    return;
+  }
+
+  auto It = FirstSpecialInsts.find(BB);
+  bool BlockHasSpecialInsns = false;
+  for (const Instruction &Insn : *BB) {
+    if (isSpecialInstruction(&Insn)) {
+      assert(It != FirstSpecialInsts.end() &&
+             "Blocked marked as known but we have no cached value for it!");
+      assert(It->second == &Insn &&
+             "Cached first special instruction is wrong!");
+      BlockHasSpecialInsns = true;
+      break;
+    }
+  }
+  if (!BlockHasSpecialInsns)
+    assert(It == FirstSpecialInsts.end() &&
+           "Block is marked as having special instructions but in fact it "
+           "has none!");
+}
+
+void InstructionPrecedenceTracking::validateAll() const {
+  // Check that for every known block the cached value is correct.
+  for (auto *BB : KnownBlocks)
+    validate(BB);
+
+  // Check that all blocks with cached values are marked as known.
+  for (auto &It : FirstSpecialInsts)
+    assert(KnownBlocks.count(It.first) &&
+           "We have a cached value but the block is not marked as known?");
+}
+#endif
+
 void InstructionPrecedenceTracking::invalidateBlock(const BasicBlock *BB) {
   OI.invalidateBlock(BB);
   FirstSpecialInsts.erase(BB);
@@ -67,6 +123,10 @@ void InstructionPrecedenceTracking::clea
     OI.invalidateBlock(It.first);
   FirstSpecialInsts.clear();
   KnownBlocks.clear();
+#ifndef NDEBUG
+  // The map should be valid after clearing (at least empty).
+  validateAll();
+#endif
 }
 
 bool ImplicitControlFlowTracking::isSpecialInstruction(