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

[Philip Reames via llvm-commits]

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
> 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/057bb07a/attachment.html>