luke
/
verilator
mirror of https://github.com/verilator/verilator.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Internals: Speed up and improve V3Broken 

This patch makes OpenTitan verilation with --debug-check 22% faster, and
the same with --debug --no-dump-tree 91% faster. Functionality is the
same (including when VL_LEAK_CHECKS is defined), except V3Broken can now
always find duplicate references via child/next pointers if the target
node is not `maybePointedTo()` (previously this only happened when
compiled with VL_LEAK_CHECKS). The main change relates to storing the
v3Broken traversal state in the AstNode by stealing a byte from what
used to be unused flags. We retain an unordered_set only for marking
pointers as valid to be referenced via a non-child/non-next member
pointer.
This commit is contained in:
Geza Lore 2021-07-10 16:13:05 +01:00
parent 8073e8bb46
commit 5ad3c4e499
3 changed files with 137 additions and 192 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
src/V3Ast.h
View File

@ -24,6 +24,7 @@
#include "V3FileLine.h" #include "V3FileLine.h"
#include "V3Number.h" #include "V3Number.h"
#include "V3Global.h" #include "V3Global.h"
#include "V3Broken.h"
#include <cmath> #include <cmath>
#include <type_traits> #include <type_traits>
@ -1335,21 +1336,29 @@ class AstNode VL_NOT_FINAL {
// ^ ASTNODE_PREFETCH depends on above ordering of members // ^ ASTNODE_PREFETCH depends on above ordering of members
// AstType is 2 bytes, so we can stick another 6 bytes after it to utilize what would // AstType is 2 bytes, so we can stick another 6 bytes after it to utilize what would
// otherwise be padding (on a 64-bit system). We stick the attribute flags and the clone // otherwise be padding (on a 64-bit system). We stick the attribute flags, broken state,
// count here. // and the clone count here.
struct { struct {
bool didWidth : 1; // Did V3Width computation bool didWidth : 1; // Did V3Width computation
bool doingWidth : 1; // Inside V3Width bool doingWidth : 1; // Inside V3Width
bool protect : 1; // Protect name if protection is on bool protect : 1; // Protect name if protection is on
uint16_t unused : 13; // Space for more flags here (there must be 16 bits in total) // Space for more flags here (there must be 8 bits in total)
uint8_t unused : 5;
} m_flags; // Attribute flags } m_flags; // Attribute flags
// State variable used by V3Broken for consistency checking. The top bit of this is byte is a
// flag, representing V3Broken is currently proceeding under this node. The bottom 7 bits are
// a generation number. This is hot when --debug-checks so we access as a whole to avoid bit
// field masking resulting in unnecessary read-modify-write ops.
uint8_t m_brokenState = 0;
int m_cloneCnt; // Sequence number for when last clone was made int m_cloneCnt; // Sequence number for when last clone was made
#if defined(__x86_64__) && defined(__gnu_linux__) #if defined(__x86_64__) && defined(__gnu_linux__)
// Only assert this on known platforms, as it only affects performance, not correctness // Only assert this on known platforms, as it only affects performance, not correctness
static_assert(sizeof(m_type) + sizeof(m_flags) + sizeof(m_cloneCnt) <= sizeof(void*), static_assert(sizeof(m_type) + sizeof(m_flags) + sizeof(m_brokenState) + sizeof(m_cloneCnt)
<= sizeof(void*),
"packing requires padding"); "packing requires padding");
#endif #endif
@ -1466,9 +1475,14 @@ public:
AstNode* firstAbovep() const { // Returns nullptr when second or later in list AstNode* firstAbovep() const { // Returns nullptr when second or later in list
return ((backp() && backp()->nextp() != this) ? backp() : nullptr); return ((backp() && backp()->nextp() != this) ? backp() : nullptr);
} }
bool brokeExists() const; uint8_t brokenState() const { return m_brokenState; }
bool brokeExistsAbove() const; void brokenState(uint8_t value) { m_brokenState = value; }
bool brokeExistsBelow() const;
// Used by AstNode::broken()
bool brokeExists() const { return V3Broken::isLinkable(this); }
bool brokeExistsAbove() const { return brokeExists() && (m_brokenState >> 7); }
bool brokeExistsBelow() const { return brokeExists() && !(m_brokenState >> 7); }
// Note: brokeExistsBelow is not quite precise, as it is true for sibling nodes as well
// CONSTRUCTORS // CONSTRUCTORS
virtual ~AstNode() = default; virtual ~AstNode() = default;

280
src/V3Broken.cpp
View File

@ -32,202 +32,129 @@
// This visitor does not edit nodes, and is called at error-exit, so should use constant iterators // This visitor does not edit nodes, and is called at error-exit, so should use constant iterators
#include "V3AstConstOnly.h" #include "V3AstConstOnly.h"
#include <algorithm>
#include <unordered_map> #include <unordered_map>
#include <unordered_set> #include <unordered_set>
//###################################################################### //######################################################################
// Generation counter for AstNode::m_brokenState
class BrokenTable VL_NOT_FINAL : public AstNVisitor { static class BrokenCntGlobal {
// Table of brokenExists node pointers // This is a 7 bit generation counter, stored in the bottom 7 bits of AstNode::m_brokenState,
// used to mark a node as being present under the root AstNetlist in the current traversal. A
// value 0 is invalid, as the AstNode constructor uses that to initialize m_brokenState
static constexpr uint8_t MIN_VALUE = 1;
static constexpr uint8_t MAX_VALUE = 127;
uint8_t m_count = MIN_VALUE;
public:
uint8_t get() {
UASSERT(MIN_VALUE <= m_count && m_count <= MAX_VALUE, "Invalid generation number");
return m_count;
}
void inc() {
++m_count;
if (m_count > MAX_VALUE) m_count = MIN_VALUE;
}
} s_brokenCntGlobal;
//######################################################################
// Table of allocated AstNode pointers
static class AllocTable final {
private: private:
// MEMBERS // MEMBERS
// For each node, we keep if it exists or not. std::unordered_set<const AstNode*> m_allocated; // Set of all nodes allocated but not freed
using NodeMap = std::unordered_map<const AstNode*, int>; // Performance matters (when --debug)
static NodeMap s_nodes; // Set of all nodes that exist
// BITMASK
enum { FLAG_ALLOCATED = 0x01 }; // new() and not delete()ed
enum { FLAG_IN_TREE = 0x02 }; // Is in netlist tree
enum { FLAG_LINKABLE = 0x04 }; // Is in netlist tree, can be linked to
enum { FLAG_LEAKED = 0x08 }; // Known to have been leaked
enum { FLAG_UNDER_NOW = 0x10 }; // Is in tree as parent of current node
public: public:
// METHODS // METHODS
static void deleted(const AstNode* nodep) { void addNewed(const AstNode* nodep) {
// Called by operator delete on any node - only if VL_LEAK_CHECKS
if (debug() >= 9) cout << "-nodeDel: " << cvtToHex(nodep) << endl;
const auto iter = s_nodes.find(nodep);
UASSERT_OBJ(!(iter == s_nodes.end() || !(iter->second & FLAG_ALLOCATED)),
reinterpret_cast<const AstNode*>(nodep),
"Deleting AstNode object that was never tracked or already deleted");
if (iter != s_nodes.end()) s_nodes.erase(iter);
}
#if defined(__GNUC__) && __GNUC__ == 4 && __GNUC_MINOR__ == 4
// GCC 4.4.* compiler warning bug, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=39390
#pragma GCC diagnostic ignored "-Wstrict-aliasing"
#endif
static void addNewed(const AstNode* nodep) {
// Called by operator new on any node - only if VL_LEAK_CHECKS // Called by operator new on any node - only if VL_LEAK_CHECKS
if (debug() >= 9) cout << "-nodeNew: " << cvtToHex(nodep) << endl;
const auto iter = s_nodes.find(nodep);
UASSERT_OBJ(!(iter != s_nodes.end() && (iter->second & FLAG_ALLOCATED)), nodep,
"Newing AstNode object that is already allocated");
if (iter == s_nodes.end()) {
const int flags = FLAG_ALLOCATED; // This int needed to appease GCC 4.1.2
s_nodes.emplace(nodep, flags);
}
}
static void setUnder(const AstNode* nodep, bool flag) {
// Called by BrokenCheckVisitor when each node entered/exited
if (!okIfLinkedTo(nodep)) return;
const auto iter = s_nodes.find(nodep);
if (iter != s_nodes.end()) {
iter->second &= ~FLAG_UNDER_NOW;
if (flag) iter->second |= FLAG_UNDER_NOW;
}
}
static void addInTree(AstNode* nodep, bool linkable) {
#ifndef VL_LEAK_CHECKS
// cppcheck-suppress knownConditionTrueFalse
if (!linkable) return; // save some time, else the map will get huge!
#endif
const auto iter = s_nodes.find(nodep);
if (VL_UNCOVERABLE(iter == s_nodes.end())) {
#ifdef VL_LEAK_CHECKS
nodep->v3fatalSrc("AstNode is in tree, but not allocated");
#endif
} else {
#ifdef VL_LEAK_CHECKS
UASSERT_OBJ(iter->second & FLAG_ALLOCATED, nodep,
"AstNode is in tree, but not allocated");
#endif
UASSERT_OBJ(!(iter->second & FLAG_IN_TREE), nodep,
"AstNode is already in tree at another location");
}
const int or_flags = FLAG_IN_TREE | (linkable ? FLAG_LINKABLE : 0);
if (iter == s_nodes.end()) {
s_nodes.emplace(nodep, or_flags);
} else {
iter->second |= or_flags;
}
}
static bool isAllocated(const AstNode* nodep) {
// Some generic node has a pointer to this node. Is it allocated?
// Use this when might not be in tree; otherwise use okIfLinkedTo().
#ifdef VL_LEAK_CHECKS
const auto iter = s_nodes.find(nodep);
if (iter == s_nodes.end()) return false;
if (!(iter->second & FLAG_ALLOCATED)) return false;
#endif
return true;
}
static bool okIfLinkedTo(const AstNode* nodep) {
// Some node in tree has a pointer to this node. Is it kosher?
const auto iter = s_nodes.find(nodep);
if (iter == s_nodes.end()) return false;
#ifdef VL_LEAK_CHECKS
if (!(iter->second & FLAG_ALLOCATED)) return false;
#endif
if (!(iter->second & FLAG_IN_TREE)) return false;
if (!(iter->second & FLAG_LINKABLE)) return false;
return true;
}
static bool okIfAbove(const AstNode* nodep) {
// Must be linked to and below current node
if (!okIfLinkedTo(nodep)) return false;
const auto iter = s_nodes.find(nodep);
if (iter == s_nodes.end()) return false;
if ((iter->second & FLAG_UNDER_NOW)) return false;
return true;
}
static bool okIfBelow(const AstNode* nodep) {
// Must be linked to and below current node
if (!okIfLinkedTo(nodep)) return false;
const auto iter = s_nodes.find(nodep);
if (iter == s_nodes.end()) return false;
if (!(iter->second & FLAG_UNDER_NOW)) return false;
return true;
}
static void prepForTree() {
#ifndef VL_LEAK_CHECKS
s_nodes.clear();
#else
for (NodeMap::iterator it = s_nodes.begin(); it != s_nodes.end(); ++it) {
it->second &= ~FLAG_IN_TREE;
it->second &= ~FLAG_LINKABLE;
}
#endif
}
static void doneWithTree() {
for (int backs = 0; backs < 2;
backs++) { // Those with backp() are probably under one leaking without
for (NodeMap::iterator it = s_nodes.begin(); it != s_nodes.end(); ++it) {
// LCOV_EXCL_START // LCOV_EXCL_START
if (VL_UNCOVERABLE((it->second & FLAG_ALLOCATED) && !(it->second & FLAG_IN_TREE) if (VL_UNCOVERABLE(!m_allocated.emplace(nodep).second)) {
&& !(it->second & FLAG_LEAKED) nodep->v3fatalSrc("Newing AstNode object that is already allocated");
&& (it->first->backp() ? backs == 1 : backs == 0))) { }
// LCOV_EXCL_STOP
}
void deleted(const AstNode* nodep) {
// Called by operator delete on any node - only if VL_LEAK_CHECKS
// LCOV_EXCL_START
if (VL_UNCOVERABLE(m_allocated.erase(nodep) == 0)) {
nodep->v3fatalSrc("Deleting AstNode object that was not allocated or already freed");
}
// LCOV_EXCL_STOP
}
bool isAllocated(const AstNode* nodep) const { return m_allocated.count(nodep) != 0; }
void checkForLeaks() {
if (!v3Global.opt.debugCheck()) return;
const uint8_t brokenCntCurrent = s_brokenCntGlobal.get();
// Those with backp() are probably under a parent that was leaked and has no backp()
for (const bool withBack : {false, true}) {
for (const AstNode* const nodep : m_allocated) {
// LCOV_EXCL_START
// Most likely not leaked, so check that first
if (VL_UNCOVERABLE(nodep->brokenState() != brokenCntCurrent)) {
const bool hasBack = nodep->backp() != nullptr;
if (hasBack != withBack) continue;
// Use only AstNode::dump instead of the virtual one, as there // Use only AstNode::dump instead of the virtual one, as there
// may be varp() and other cross links that are bad. // may be varp() and other cross links that are bad.
if (v3Global.opt.debugCheck()) {
// When get this message, find what forgot to delete the // When get this message, find what forgot to delete the
// node by running GDB, where for node "<e###>" use: // node by running GDB, where for node "<e###>" use:
// watch AstNode::s_editCntGbl==#### // watch AstNode::s_editCntGbl==####
// run // run
// bt // bt
std::cerr << "%Error: LeakedNode" std::cerr << "%Error: LeakedNode" << (withBack ? "with back pointer: " : ": ");
<< (it->first->backp() ? "Back: " : ": "); nodep->AstNode::dump(std::cerr);
AstNode* rawp
= const_cast<AstNode*>(static_cast<const AstNode*>(it->first));
rawp->AstNode::dump(std::cerr);
std::cerr << endl; std::cerr << endl;
V3Error::incErrors(); V3Error::incErrors();
} }
it->second |= FLAG_LEAKED;
}
// LCOV_EXCL_STOP // LCOV_EXCL_STOP
} }
} }
} }
} s_allocTable;
// CONSTRUCTORS void V3Broken::addNewed(const AstNode* nodep) { s_allocTable.addNewed(nodep); }
BrokenTable() = default; void V3Broken::deleted(const AstNode* nodep) { s_allocTable.deleted(nodep); }
virtual ~BrokenTable() override = default;
};
BrokenTable::NodeMap BrokenTable::s_nodes;
bool AstNode::brokeExists() const {
// Called by node->broken() routines to do table lookup
return BrokenTable::okIfLinkedTo(this);
}
bool AstNode::brokeExistsAbove() const {
// Called by node->broken() routines to do table lookup
return BrokenTable::okIfBelow(this);
}
bool AstNode::brokeExistsBelow() const {
// Called by node->broken() routines to do table lookup
return BrokenTable::okIfAbove(this);
}
//###################################################################### //######################################################################
// Table of AstNode pointers that can be linked to via member pointers
static class LinkableTable final {
private:
// MEMBERS
std::unordered_set<const AstNode*> m_linkable; // Set of all nodes allocated but not freed
public:
// METHODS
void clear() { m_linkable.clear(); }
inline void addLinkable(const AstNode* nodep) { m_linkable.emplace(nodep); }
inline bool isLinkable(const AstNode* nodep) const { return m_linkable.count(nodep) != 0; }
} s_linkableTable;
bool V3Broken::isLinkable(const AstNode* nodep) { return s_linkableTable.isLinkable(nodep); }
//######################################################################
// Mark every node in the tree
class BrokenMarkVisitor final : public AstNVisitor { class BrokenMarkVisitor final : public AstNVisitor {
// Mark every node in the tree
private: private:
// NODE STATE const uint8_t m_brokenCntCurrent = s_brokenCntGlobal.get();
// Nothing! // This may be called deep inside other routines
// // so userp and friends may not be used
// METHODS
void processAndIterate(AstNode* nodep) {
BrokenTable::addInTree(nodep, nodep->maybePointedTo());
iterateChildrenConst(nodep);
}
// VISITORS // VISITORS
virtual void visit(AstNode* nodep) override { virtual void visit(AstNode* nodep) override {
// Process not just iterate #ifdef VL_LEAK_CHECKS
processAndIterate(nodep); UASSERT_OBJ(s_allocTable.isAllocated(nodep), nodep,
"AstNode is in tree, but not allocated");
#endif
UASSERT_OBJ(nodep->brokenState() != m_brokenCntCurrent, nodep,
"AstNode is already in tree at another location");
if (nodep->maybePointedTo()) s_linkableTable.addLinkable(nodep);
nodep->brokenState(m_brokenCntCurrent);
iterateChildrenConst(nodep);
} }
public: public:
@ -237,11 +164,15 @@ public:
}; };
//###################################################################### //######################################################################
// Broken state, as a visitor of each AstNode // Check every node in tree
class BrokenCheckVisitor final : public AstNVisitor { class BrokenCheckVisitor final : public AstNVisitor {
bool m_inScope = false; // Under AstScope bool m_inScope = false; // Under AstScope
// Constants for marking we are under/not under a node
const uint8_t m_brokenCntCurrentNotUnder = s_brokenCntGlobal.get(); // Top bit is clear
const uint8_t m_brokenCntCurrentUnder = m_brokenCntCurrentNotUnder | 0x80; // Top bit is set
// Current CFunc, if any // Current CFunc, if any
const AstCFunc* m_cfuncp = nullptr; const AstCFunc* m_cfuncp = nullptr;
// All local variables declared in current function // All local variables declared in current function
@ -259,7 +190,7 @@ private:
} }
void processEnter(AstNode* nodep) { void processEnter(AstNode* nodep) {
BrokenTable::setUnder(nodep, true); nodep->brokenState(m_brokenCntCurrentUnder);
const char* whyp = nodep->broken(); const char* whyp = nodep->broken();
UASSERT_OBJ(!whyp, nodep, UASSERT_OBJ(!whyp, nodep,
"Broken link in node (or something without maybePointedTo): " << whyp); "Broken link in node (or something without maybePointedTo): " << whyp);
@ -283,7 +214,7 @@ private:
} }
checkWidthMin(nodep); checkWidthMin(nodep);
} }
void processExit(AstNode* nodep) { BrokenTable::setUnder(nodep, false); } void processExit(AstNode* nodep) { nodep->brokenState(m_brokenCntCurrentNotUnder); }
void processAndIterate(AstNode* nodep) { void processAndIterate(AstNode* nodep) {
processEnter(nodep); processEnter(nodep);
iterateChildrenConst(nodep); iterateChildrenConst(nodep);
@ -401,7 +332,7 @@ public:
}; };
//###################################################################### //######################################################################
// Broken class functions // Broken check entry point
void V3Broken::brokenAll(AstNetlist* nodep) { void V3Broken::brokenAll(AstNetlist* nodep) {
// UINFO(9, __FUNCTION__ << ": " << endl); // UINFO(9, __FUNCTION__ << ": " << endl);
@ -411,24 +342,23 @@ void V3Broken::brokenAll(AstNetlist* nodep) {
UINFO(1, "Broken called under broken, skipping recursion.\n"); // LCOV_EXCL_LINE UINFO(1, "Broken called under broken, skipping recursion.\n"); // LCOV_EXCL_LINE
} else { } else {
inBroken = true; inBroken = true;
BrokenTable::prepForTree();
BrokenMarkVisitor mvisitor(nodep); BrokenMarkVisitor mvisitor(nodep);
BrokenCheckVisitor cvisitor(nodep); BrokenCheckVisitor cvisitor(nodep);
BrokenTable::doneWithTree(); s_allocTable.checkForLeaks();
s_linkableTable.clear();
s_brokenCntGlobal.inc();
inBroken = false; inBroken = false;
} }
} }
void V3Broken::addNewed(AstNode* nodep) { BrokenTable::addNewed(nodep); } //######################################################################
void V3Broken::deleted(AstNode* nodep) { BrokenTable::deleted(nodep); } // Self test
bool V3Broken::isAllocated(AstNode* nodep) { return BrokenTable::isAllocated(nodep); }
void V3Broken::selfTest() { void V3Broken::selfTest() {
// Warmup addNewed and deleted for coverage, as otherwise only with VL_LEAK_CHECKS // Exercise addNewed and deleted for coverage, as otherwise only used with VL_LEAK_CHECKS
FileLine* fl = new FileLine(FileLine::commandLineFilename()); FileLine* const fl = new FileLine(FileLine::commandLineFilename());
auto* newp = new AstBegin(fl, "[EditWrapper]", nullptr); const AstNode* const newp = new AstBegin(fl, "[EditWrapper]", nullptr);
#ifndef VL_LEAK_CHECKS
addNewed(newp); addNewed(newp);
deleted(newp); deleted(newp);
#endif
VL_DO_DANGLING(delete newp, newp); VL_DO_DANGLING(delete newp, newp);
} }

13
src/V3Broken.h
View File

@ -20,17 +20,18 @@
#include "config_build.h" #include "config_build.h"
#include "verilatedos.h" #include "verilatedos.h"
#include "V3Error.h"
#include "V3Ast.h"
//============================================================================ //============================================================================
// Forward declare so we can include this in V3Ast.h
class AstNode;
class AstNetlist;
class V3Broken final { class V3Broken final {
public: public:
static void brokenAll(AstNetlist* nodep); static void brokenAll(AstNetlist* nodep);
static void addNewed(AstNode* nodep); static bool isLinkable(const AstNode* nodep);
static void deleted(AstNode* nodep); static void addNewed(const AstNode* nodep);
static bool isAllocated(AstNode* nodep); static void deleted(const AstNode* nodep);
static void selfTest(); static void selfTest();
}; };