[llvm] r341051 - [NFC] Add severe validation of InstructionPrecedenceTracking

[Maxim Kazantsev via llvm-commits]

Hi Philip,

Let me try to address your concerns.


·         We do not expect *all* blocks to be populated all the time and never check it. We do expect that for all blocks we claim to have the cached answer this answer is correct.

·         Agreed, but it only happens in debug mode.

·         We don't have to have a correct state on destructor. We could have changed the block contents without bothering to invalidate the ICF because we know that we will no longer make any queries to it. Therefore, the validation on destructor doesn’t sound.

If you still think it should be reverted, I'll do it.

Thanks,
Max

From: Philip Reames [mailto:listmail at philipreames.com]
Sent: Thursday, August 30, 2018 11:16 PM
To: Maxim Kazantsev <max.kazantsev at azul.com>; llvm-commits at lists.llvm.org
Subject: Re: [llvm] r341051 - [NFC] Add severe validation of InstructionPrecedenceTracking


Max,

I see a couple of problems with this patch, please revert so that we can discuss in a review thread.

Problems:

  *   The implemented validation appears to expect all blocks with special instructions to be populated at all times.  This appears inconsistent the lazy semantics elsewhere.
  *   The validation on getFirstSpecialInstruction is too heavyweight.  We need a checker check there.
  *   We're not validating on destruction which is the entire point.

Philip

On 08/30/2018 03:26 AM, Max Kazantsev via llvm-commits wrote:

Author: mkazantsev

Date: Thu Aug 30 03:26:06 2018

New Revision: 341051



URL: http://llvm.org/viewvc/llvm-project?rev=341051&view=rev

Log:

[NFC] Add severe validation of InstructionPrecedenceTracking



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=341051&r1=341050&r2=341051&view=diff

==============================================================================

--- llvm/trunk/include/llvm/Analysis/InstructionPrecedenceTracking.h (original)

+++ llvm/trunk/include/llvm/Analysis/InstructionPrecedenceTracking.h Thu Aug 30 03:26:06 2018

@@ -37,6 +37,15 @@ class InstructionPrecedenceTracking {

   // Fills information about the given block's special instructions.

   void fill(const BasicBlock *BB);



+  #ifndef NDEBUG

+  /// Asserts whether or not the contents of this tracking is up-to-date. It can

+  /// be used to detect situations where we failed to invalidate the map

+  /// properly. The behavior of request to an invalid tracking is undefined, and

+  /// we should avoid such situations. It is slow and should only be called in

+  /// debug mode.

+  void validate() 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=341051&r1=341050&r2=341051&view=diff

==============================================================================

--- llvm/trunk/lib/Analysis/InstructionPrecedenceTracking.cpp (original)

+++ llvm/trunk/lib/Analysis/InstructionPrecedenceTracking.cpp Thu Aug 30 03:26:06 2018

@@ -25,6 +25,11 @@ using namespace llvm;



 const Instruction *InstructionPrecedenceTracking::getFirstSpecialInstruction(

     const BasicBlock *BB) {

+#ifndef NDEBUG

+  // Make sure that we are making this request to tracking which is up-to-date.

+  validate();

+#endif

+

   if (!KnownBlocks.count(BB))

     fill(BB);

   auto *FirstICF = FirstSpecialInsts.lookup(BB);

@@ -56,6 +61,36 @@ void InstructionPrecedenceTracking::fill

   KnownBlocks.insert(BB);

 }



+#ifndef NDEBUG

+void InstructionPrecedenceTracking::validate() const {

+  unsigned NumNoSpecialBlocks = 0;

+  // Check that for every known block we have something cached for it.

+  for (auto *BB : KnownBlocks) {

+    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!");

+      ++NumNoSpecialBlocks;

+    }

+  }

+

+  assert(KnownBlocks.size() == NumNoSpecialBlocks + FirstSpecialInsts.size() &&

+         "We don't have info for some blocks?");

+}

+#endif

+

 void InstructionPrecedenceTracking::invalidateBlock(const BasicBlock *BB) {

   OI.invalidateBlock(BB);

   FirstSpecialInsts.erase(BB);

@@ -67,6 +102,10 @@ void InstructionPrecedenceTracking::clea

     OI.invalidateBlock(It.first);

   FirstSpecialInsts.clear();

   KnownBlocks.clear();

+#ifndef NDEBUG

+  // The map should be valid after clearing (at least empty).

+  validate();

+#endif

 }



 bool ImplicitControlFlowTracking::isSpecialInstruction(





_______________________________________________

llvm-commits mailing list

llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>

http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180830/3ae23171/attachment-0001.html>