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

[Philip Reames via llvm-commits]

On 09/06/2018 01:33 AM, Max Kazantsev via llvm-commits wrote:
> 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.
Max, the wording here didn't quite mean what you meant.  We already had 
the discussion, this patch isn't intended to trigger one.  :)
>
> 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(
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits