<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Max,</p>
<p>I see a couple of problems with this patch, please revert so that
we can discuss in a review thread.</p>
<p>Problems:</p>
<ul>
<li>The implemented validation appears to expect all blocks with
special instructions to be populated at all times. This appears
inconsistent the lazy semantics elsewhere.</li>
<li>The validation on getFirstSpecialInstruction is too
heavyweight. We need a checker check there.</li>
<li>We're not validating on destruction which is the entire point.</li>
</ul>
<p>Philip<br>
</p>
<br>
<div class="moz-cite-prefix">On 08/30/2018 03:26 AM, Max Kazantsev
via llvm-commits wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20180830102606.B9BB088A89@lists.llvm.org">
<pre wrap="">Author: mkazantsev
Date: Thu Aug 30 03:26:06 2018
New Revision: 341051
URL: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project?rev=341051&view=rev">http://llvm.org/viewvc/llvm-project?rev=341051&view=rev</a>
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: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/InstructionPrecedenceTracking.h?rev=341051&r1=341050&r2=341051&view=diff">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/InstructionPrecedenceTracking.h?rev=341051&r1=341050&r2=341051&view=diff</a>
==============================================================================
--- 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: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionPrecedenceTracking.cpp?rev=341051&r1=341050&r2=341051&view=diff">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionPrecedenceTracking.cpp?rev=341051&r1=341050&r2=341051&view=diff</a>
==============================================================================
--- 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
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</body>
</html>