luke/verilator
1
0
Fork 0
mirror of https://github.com/verilator/verilator.git synced 2026-10-06 10:03:44 +02:00
Code Issues Packages Projects Releases Wiki Activity

Fix folding of short circuiting operators guarding member access (#8238)

Browse Source 
V3Const eagerly deletes nodes (we want that in V3Const instead of using
pushDeletep, as otherwise the transient peak memory use during
verilation can be very high)

The std::map used to cache the predicate checking if a subtree contains
a member access then stale if a new node is allocated in the same address.

Fix by using unique node IDs stored in user4 as cache keys.

This is the bug uncovered by #8147

Related to #6963
This commit is contained in:
Geza Lore
2026-08-28 20:16:03 +02:00
committed by GitHub
parent 6a05cc680b
commit 297713df8b
4 changed files with 311 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/V3Const.cpp
+68 -45
View File
@@ -38,6 +38,7 @@
#include <algorithm>
#include <memory>
#include <type_traits>
#include <unordered_map>
#include <unordered_set>
VL_DEFINE_DEBUG_FUNCTIONS;
@@ -78,15 +79,29 @@ static int countTrailingZeroes(uint64_t val) {
#endif
}
// Allocates a unique id for each AstNode it is applied to, held in user4.
class VNIdAllocator final {
// NODE STATE
// AstNode::user4p -> size_t. Unique id of the node (0/nullptr if not allocated yet)
// MEMBERS
// Claimed lazily on first use TODO: fix conflict with V3Param user4 slot
std::unique_ptr<VNUser4InUse> m_inuser4p;
size_t m_nextId = 0; // Id allocated most recently
public:
// Return the unique id of the given node, allocating a new one if it has none yet
size_t operator()(AstNode* nodep) {
if (!m_inuser4p) m_inuser4p = std::make_unique<VNUser4InUse>(); // Claim on first use
if (!nodep->user4p()) nodep->user4p(reinterpret_cast<void*>(++m_nextId));
return reinterpret_cast<size_t>(nodep->user4p());
}
};
// This visitor can be used in the post-expanded Ast from V3Expand, where the Ast satisfies:
// - Constants are 64 bit at most (because words are accessed via AstWordSel)
// - Variables are scoped.
class ConstBitOpTreeVisitor final : public VNVisitorConst {
// NODE STATE
// AstVarRef::user4u -> Base index of m_varInfos that points VarInfo
// AstVarScope::user4u -> Same as AstVarRef::user4
const VNUser4InUse m_inuser4;
// TYPES
// Holds a node to be added as a term in the reduction tree, it's equivalent op count, and a
@@ -382,6 +397,10 @@ class ConstBitOpTreeVisitor final : public VNVisitorConst {
m_frozenNodes; // Nodes that cannot be optimized
std::vector<BitPolarityEntry> m_bitPolarities; // Polarity of bits found during iterate()
std::vector<std::unique_ptr<VarInfo>> m_varInfos; // VarInfo for each variable, [0] is nullptr
VNIdAllocator& m_ids; // Node id allocator, owned by ConstVisitor
// Base index of m_varInfos that points VarInfo, keyed by AstVarScope/AstVarRef id,
// zero means not set yet.
std::unordered_map<size_t, int> m_baseIdxs;
// METHODS
@@ -411,15 +430,14 @@ class ConstBitOpTreeVisitor final : public VNVisitorConst {
UASSERT_OBJ(ref.refp(), m_rootp, "null varref in And/Or/Xor optimization");
AstNode* nodep = ref.refp()->varScopep();
if (!nodep) nodep = ref.refp()->varp(); // Not scoped
int baseIdx = nodep->user4();
if (baseIdx == 0) { // Not set yet
baseIdx = m_varInfos.size();
int& baseIdxr = m_baseIdxs[m_ids(nodep)];
if (baseIdxr == 0) { // Not set yet
baseIdxr = m_varInfos.size();
const int numWords
= ref.refp()->dtypep()->isWide() ? ref.refp()->dtypep()->widthWords() : 1;
m_varInfos.resize(m_varInfos.size() + numWords);
nodep->user4(baseIdx);
}
const size_t idx = baseIdx + std::max(0, ref.wordIdx());
const size_t idx = baseIdxr + std::max(0, ref.wordIdx());
VarInfo* varInfop = m_varInfos[idx].get();
if (!varInfop) {
varInfop = new VarInfo{this, ref.refp(), ref.varWidth()};
@@ -671,10 +689,11 @@ class ConstBitOpTreeVisitor final : public VNVisitorConst {
}
// CONSTRUCTORS
ConstBitOpTreeVisitor(AstNodeExpr* nodep, unsigned externalOps)
ConstBitOpTreeVisitor(AstNodeExpr* nodep, unsigned externalOps, VNIdAllocator& ids)
: m_ops{externalOps}
, m_rootp{nodep} {
// Fill nullptr at [0] because AstVarScope::user4 is 0 by default
, m_rootp{nodep}
, m_ids{ids} {
// Fill nullptr at [0] because a base index of 0 means not set yet
m_varInfos.push_back(nullptr);
CONST_BITOP_RETURN_IF(!isAndTree() && !isOrTree() && !isXorTree(), nodep);
if (AstNodeBiop* const biopp = VN_CAST(nodep, NodeBiop)) {
@@ -703,11 +722,11 @@ public:
// Reduction ops are transformed in the same way.
// &{v[0], v[1]} => 2'b11 == (2'b11 & v)
static AstNodeExpr* simplify(AstNodeExpr* nodep, int resultWidth, unsigned externalOps,
VDouble0& reduction) {
VDouble0& reduction, VNIdAllocator& ids) {
UASSERT_OBJ(1 <= resultWidth && resultWidth <= 64, nodep, "resultWidth out of range");
// Walk tree, gathering all terms referenced in expression
const ConstBitOpTreeVisitor visitor{nodep, externalOps};
const ConstBitOpTreeVisitor visitor{nodep, externalOps, ids};
// If failed on root node is not optimizable, or there are no variable terms, then done
if (visitor.m_failed || visitor.m_varInfos.size() == 1) return nullptr;
@@ -913,12 +932,11 @@ class ConstVisitor final : public VNVisitor {
static constexpr unsigned CONCAT_MERGABLE_MAX_DEPTH = 10; // Limit alg recursion
// NODE STATE
// ** only when m_warn/m_doExpensive is set. If state is needed other times,
// ** must track down everywhere V3Const is called and make sure no overlaps.
// AstVar::user4p -> Used by variable marking/finding
// AstEnum::user4 -> bool. Recursing.
// See VNIdAllocator. All per node state below is held in side tables keyed on the
// node ids it hands out, as node pointers are not stable keys within V3Const.
// STATE
VNIdAllocator m_ids; // Node id allocator
bool m_params = false; // If true, propagate parameterized and true numbers only
bool m_required = false; // If true, must become a constant
bool m_wremove = true; // Inside scope, no assignw removal
@@ -940,10 +958,19 @@ class ConstVisitor final : public VNVisitor {
VDouble0 m_statConcatMerge; // Concat merges
VDouble0 m_statCondExprRedundant; // Conditional repeated expressions
VDouble0 m_statIfCondExprRedundant; // Conditional repeated expressions
VDouble0 m_statMemberAccessVisits; // Nodes visited by containsMemberAccessRecurse
const bool m_globalPass; // ConstVisitor invoked as a global pass
static uint32_t s_globalPassNum; // Counts number of times ConstVisitor invoked as global pass
V3UniqueNames m_concswapNames; // For generating unique temporary variable names
std::map<const AstNode*, bool> m_containsMemberAccess; // Caches results of matchBiopToBitwise
// Caches results of matchBiopToBitwise, keyed on node id
std::unordered_map<size_t, bool> m_containsMemberAccess;
// Items of enum currently being recursed into. Cannot use VNIdAllocator, as this runs in
// every mode, including those invoked under a pass holding user4 (V3Param). Node pointers
// are safe as keys here, unlike elsewhere in V3Const: folding the item value below does
// free nodes whose addresses are then reused, but AstEnumItem is only ever constructed
// while parsing (verilog.y and V3LinkParse), so a reused address can never become one,
// and every key looked up is an AstEnumItem.
std::unordered_set<const AstEnumItem*> m_recursingEnumItems;
std::unordered_set<AstJumpBlock*> m_usedJumpBlocks; // JumpBlocks used by some JumpGo
// METHODS
@@ -1485,8 +1512,8 @@ class ConstVisitor final : public VNVisitor {
nodep->dumpTree(debugPrefix + "INPUT: ");
} // LCOV_EXCL_STOP
AstNodeExpr* const newp
= ConstBitOpTreeVisitor::simplify(rootp, width, externalOps, m_statBitOpReduction);
AstNodeExpr* const newp = ConstBitOpTreeVisitor::simplify(rootp, width, externalOps,
m_statBitOpReduction, m_ids);
if (newp) {
nodep->replaceWithKeepDType(newp);
@@ -2442,17 +2469,15 @@ class ConstVisitor final : public VNVisitor {
const bool need_temp_pure = !nodep->rhsp()->isPure();
if (m_warn && !VN_IS(nodep, AssignDly)
&& !need_temp_pure) { // Is same var on LHS and RHS?
// Note only do this (need user4) when m_warn, which is
// done as unique visitor
// If the rhs is not pure, we need a temporary variable anyway
const VNUser4InUse m_inuser4;
nodep->lhsp()->foreach([](const AstVarRef* nodep) {
UASSERT_OBJ(nodep->varp(), nodep, "Unlinked VarRef");
nodep->varp()->user4(1);
std::unordered_set<size_t> lhsVarIds;
nodep->lhsp()->foreach([&](const AstVarRef* refp) {
UASSERT_OBJ(refp->varp(), refp, "Unlinked VarRef");
lhsVarIds.emplace(m_ids(refp->varp()));
});
nodep->rhsp()->foreach([&need_temp](const AstVarRef* nodep) {
UASSERT_OBJ(nodep->varp(), nodep, "Unlinked VarRef");
if (nodep->varp()->user4()) need_temp = true;
nodep->rhsp()->foreach([&](const AstVarRef* refp) {
UASSERT_OBJ(refp->varp(), refp, "Unlinked VarRef");
if (lhsVarIds.count(m_ids(refp->varp()))) need_temp = true;
});
}
if (need_temp_pure) {
@@ -2932,10 +2957,12 @@ class ConstVisitor final : public VNVisitor {
iterate(nodep); // Again?
}
bool containsMemberAccessRecurse(const AstNode* const nodep) {
bool containsMemberAccessRecurse(AstNode* const nodep) {
if (!nodep) return false;
const auto it = m_containsMemberAccess.lower_bound(nodep);
if (it != m_containsMemberAccess.end() && it->first == nodep) return it->second;
const size_t id = m_ids(nodep);
const auto it = m_containsMemberAccess.find(id);
if (it != m_containsMemberAccess.end()) return it->second;
++m_statMemberAccessVisits;
bool result = false;
if (VN_IS(nodep, MemberSel) || VN_IS(nodep, MethodCall) || VN_IS(nodep, CMethodCall)) {
result = true;
@@ -2960,7 +2987,7 @@ class ConstVisitor final : public VNVisitor {
&& containsMemberAccessRecurse(nodep->nextp())) {
result = true;
}
m_containsMemberAccess.insert(it, std::make_pair(nodep, result));
m_containsMemberAccess.emplace(id, result);
return result;
}
@@ -3428,12 +3455,13 @@ class ConstVisitor final : public VNVisitor {
bool did = false;
if (nodep->itemp()->valuep()) {
// UINFOTREE(1, nodep->itemp()->valuep(), "", "visitvaref");
if (nodep->itemp()->user4()) {
const AstEnumItem* const itemp = nodep->itemp();
if (m_recursingEnumItems.count(itemp)) {
nodep->v3error("Recursive enum value: " << nodep->itemp()->prettyNameQ());
} else {
nodep->itemp()->user4(true);
m_recursingEnumItems.emplace(itemp);
iterateAndNextNull(nodep->itemp()->valuep());
nodep->itemp()->user4(false);
m_recursingEnumItems.erase(itemp);
}
if (AstConst* const valuep = VN_CAST(nodep->itemp()->valuep(), Const)) {
const V3Number& num = valuep->num();
@@ -3582,13 +3610,6 @@ class ConstVisitor final : public VNVisitor {
// SENGATE(SENITEM(x)) -> SENITEM(x), then let it collapse with the
// other SENITEM(x).
// Mark x in SENITEM(x)
for (AstSenItem* senp = nodep->sensesp(); senp; senp = VN_AS(senp->nextp(), SenItem)) {
if (senp->varrefp() && senp->varrefp()->varScopep()) {
senp->varrefp()->varScopep()->user4(1);
}
}
// Pass 1: Sort the sensitivity items so "posedge a or b" and "posedge b or a" and
// similar, optimizable expressions end up next to each other.
for (AstSenItem *nextp, *senp = nodep->sensesp(); senp; senp = nextp) {
@@ -4671,6 +4692,8 @@ public:
V3Stats::addStatSum("Optimizations, If cond redundant expressions",
m_statIfCondExprRedundant);
V3Stats::addStatSum("Optimizations, Concat merges", m_statConcatMerge);
V3Stats::addStatSum("Optimizations, Member access predicate node visits",
m_statMemberAccessVisits);
}
AstNode* mainAcceptEdit(AstNode* nodep) {