[llvm] r341051 - [NFC] Add severe validation of InstructionPrecedenceTracking
Maxim Kazantsev via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 30 17:07:25 PDT 2018
Let's discuss here https://reviews.llvm.org/D51523
From: Maxim Kazantsev
Sent: Friday, August 31, 2018 6:29 AM
To: 'Philip Reames' <listmail at philipreames.com>; llvm-commits at lists.llvm.org
Subject: RE: [llvm] r341051 - [NFC] Add severe validation of InstructionPrecedenceTracking
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<mailto:max.kazantsev at azul.com>>; llvm-commits at lists.llvm.org<mailto: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/20180831/0c4f0d81/attachment.html>
More information about the llvm-commits
mailing list