[llvm] r341051 - [NFC] Add severe validation of InstructionPrecedenceTracking
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 30 09:15:38 PDT 2018
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>
More information about the llvm-commits
mailing list