From 56b0afd4f89364450710ac85b55d54a047c10be7 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sat, 21 Oct 2023 23:29:02 +0100 Subject: [PATCH] Make node iteration stricter This is a prep patch for an experiment on removing m_iterpp (which I have in a hacked up form, and does not actually help much, 1.2% memory saving, yay...). Even if we don't land that, I think we want these changes that makes iteration a little bit stricter. Notably, there are assertions (only under VL_DEBUG, due to being in hot code), that ensure that we don't recursively iterate the same node. We used to do this in a few places (which I fixed), but this is generally not safe due to the recursive all to iterateAndNext overwriting m_iterpp, and hence any subsequent edits would be unsafe. Apart from the direct recursion, we also need to prevent addHereThisAsNext from updating the iteration pointer. I think this is a good thing. Previously it used to force iteration to restart on the nodes inserted *before* the currently iterated node, which would also mean that the currently iterated node would then be visited again, as it became a successor of the inserted node. With this patch, iteration continues with the edited node only if it's replacing the current node, or if the new node is a successor (nextp) of the current node, but not if the edit is adding the node before the current node. I fixed the few places (V3Premit, V3Task) where we used to rely on the old behaviour. The fix is simply to explicitly iterate the new nodes you insert before the current node. Patch is performance neutral. --- src/V3Ast.cpp | 61 ++++++++++++++++++++++++------------------------ src/V3Const.cpp | 35 ++++++++++++++------------- src/V3Premit.cpp | 37 +++++++++++++++-------------- src/V3Task.cpp | 12 ++++++---- src/V3Width.cpp | 16 ++++++++----- 5 files changed, 85 insertions(+), 76 deletions(-) diff --git a/src/V3Ast.cpp b/src/V3Ast.cpp index f7441655c5..69cdc9b12d 100644 --- a/src/V3Ast.cpp +++ b/src/V3Ast.cpp @@ -774,12 +774,6 @@ void AstNode::addHereThisAsNext(AstNode* newp) { newp->m_headtailp = tailp; tailp->m_headtailp = newp; } - // Iterator fixup - if (newLastp->m_iterpp) *(newLastp->m_iterpp) = this; - if (this->m_iterpp) { - *(this->m_iterpp) = newp; - this->m_iterpp = nullptr; - } // debugTreeChange(this, "-addHereThisAsNext: ", __LINE__, true); } @@ -858,6 +852,7 @@ AstNode* AstNode::cloneTree(bool cloneNextLink, bool needPure) { void AstNode::deleteNode() { // private: Delete single node. Publicly call deleteTree() instead. UASSERT(!m_backp, "Delete called on node with backlink still set"); + UASSERT(!m_iterpp, "Delete called on node with iterpp still set"); editCountInc(); // Change links of old node so we coredump if used this->m_nextp = reinterpret_cast(0x1); @@ -958,35 +953,39 @@ void AstNode::iterateAndNext(VNVisitor& v) { // This is a very hot function // IMPORTANT: If you replace a node that's the target of this iterator, // then the NEW node will be iterated on next, it isn't skipped! - // Future versions of this function may require the node to have a back to be iterated; - // there's no lower level reason yet though the back must exist. - AstNode* nodep = this; -#ifdef VL_DEBUG // Otherwise too hot of a function for debug - UASSERT_OBJ(!(nodep && !nodep->m_backp), nodep, "iterateAndNext node has no back"); + +#ifdef VL_DEBUG + UASSERT_OBJ(m_backp, this, "iterateAndNext called on node which has no m_backp"); + UASSERT_OBJ(!m_iterpp, this, "iterateAndNext called on node already under iteration"); #endif - // cppcheck-suppress knownConditionTrueFalse - if (nodep) ASTNODE_PREFETCH(nodep->m_nextp); - while (nodep) { // effectively: if (!this) return; // Callers rely on this + ASTNODE_PREFETCH(m_nextp); + + AstNode* nodep = this; + do { + if (nodep->m_nextp) ASTNODE_PREFETCH(nodep->m_nextp->m_nextp); - AstNode* niterp = nodep; // Pointer may get stomped via m_iterpp if the node is edited - // Desirable check, but many places where multiple iterations are OK - // UASSERT_OBJ(!niterp->m_iterpp, niterp, "IterateAndNext under iterateAndNext may miss - // edits"); Optimization note: Doing PREFETCH_RW on m_iterpp is a net even - // cppcheck-suppress nullPointer - niterp->m_iterpp = &niterp; - niterp->accept(v); - // accept may do a replaceNode and change niterp on us... - // niterp maybe nullptr, so need cast if printing - // if (niterp != nodep) UINFO(1,"iterateAndNext edited "<m_iterpp = nullptr; - if (VL_UNLIKELY(niterp != nodep)) { // Edited node inside accept + + // This pointer may get changed via m_iterpp if the current node is edited + AstNode* niterp = nodep; + nodep->m_iterpp = &niterp; + nodep->accept(v); + + if (VL_UNLIKELY(niterp != nodep)) { + // Node edited inside 'accept' (may have been deleted entirely) nodep = niterp; - } else { // Unchanged node (though maybe updated m_next), just continue loop - nodep = niterp->m_nextp; +#ifdef VL_DEBUG + UASSERT_OBJ(!nodep || (nodep->m_iterpp == &niterp), nodep, + "New node already being iterated elsewhere"); +#endif + } else { + // Unchanged node (though maybe updated m_next), just continue loop + nodep->m_iterpp = nullptr; + nodep = nodep->m_nextp; +#ifdef VL_DEBUG + UASSERT_OBJ(!nodep || !nodep->m_iterpp, niterp, "Next node already being iterated"); +#endif } - } + } while (nodep); } void AstNode::iterateListBackwardsConst(VNVisitorConst& v) { diff --git a/src/V3Const.cpp b/src/V3Const.cpp index bb68a436bf..2b527bc5fb 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -887,7 +887,7 @@ class ConstVisitor final : public VNVisitor { // ** must track down everywhere V3Const is called and make sure no overlaps. // AstVar::user4p -> Used by variable marking/finding // AstJumpLabel::user4 -> bool. Set when AstJumpGo uses this label - // AstEnum::user4 -> bool. Recursing. + // AstEnumItem::user4 -> bool. Already checked for self references // STATE static constexpr bool m_doShort = true; // Remove expressions that short circuit @@ -2744,26 +2744,29 @@ class ConstVisitor final : public VNVisitor { } void visit(AstEnumItemRef* nodep) override { iterateChildren(nodep); - UASSERT_OBJ(nodep->itemp(), nodep, "Not linked"); - bool did = false; - if (nodep->itemp()->valuep()) { - // if (debug()) nodep->itemp()->valuep()->dumpTree("- visitvaref: "); - if (nodep->itemp()->user4()) { - nodep->v3error("Recursive enum value: " << nodep->itemp()->prettyNameQ()); + AstEnumItem* const itemp = nodep->itemp(); + UASSERT_OBJ(itemp, nodep, "Not linked"); + if (!itemp->user4SetOnce() && itemp->valuep()) { + // Check for recursive self references (this should really not be here..) + const bool isRecursive = itemp->valuep()->exists([&](AstEnumItemRef* refp) { // + return refp->itemp() == itemp; + }); + if (isRecursive) { + nodep->v3error("Recursive enum value: " << itemp->prettyNameQ()); } else { - nodep->itemp()->user4(true); - iterateAndNextNull(nodep->itemp()->valuep()); - nodep->itemp()->user4(false); - } - if (AstConst* const valuep = VN_CAST(nodep->itemp()->valuep(), Const)) { - const V3Number& num = valuep->num(); - VL_DO_DANGLING(replaceNum(nodep, num), nodep); - did = true; + // Simplify the value + iterate(itemp->valuep()); } } + bool did = false; + if (AstConst* const valuep = VN_CAST(itemp->valuep(), Const)) { + const V3Number& num = valuep->num(); + VL_DO_DANGLING(replaceNum(nodep, num), nodep); + did = true; + } if (!did && m_required) { nodep->v3error("Expecting expression to be constant, but enum value isn't const: " - << nodep->itemp()->prettyNameQ()); + << itemp->prettyNameQ()); } } diff --git a/src/V3Premit.cpp b/src/V3Premit.cpp index c6ebaaceeb..574f99055b 100644 --- a/src/V3Premit.cpp +++ b/src/V3Premit.cpp @@ -93,22 +93,6 @@ class PremitVisitor final : public VNVisitor { } } - void insertBeforeStmt(AstNode* newp) { - // Insert newp before m_stmtp - if (m_inWhilep) { - // Statements that are needed for the 'condition' in a while - // actually have to be put before & after the loop, since we - // can't do any statements in a while's (cond). - m_inWhilep->addPrecondsp(newp); - } else if (m_inTracep) { - m_inTracep->addPrecondsp(newp); - } else if (m_stmtp) { - m_stmtp->addHereThisAsNext(newp); - } else { - newp->v3fatalSrc("No statement insertion point."); - } - } - void createDeepTemp(AstNodeExpr* nodep, bool noSubst) { if (nodep->user1SetOnce()) return; // Only add another assignment for this node @@ -117,6 +101,7 @@ class PremitVisitor final : public VNVisitor { FileLine* const fl = nodep->fileline(); AstVar* varp = nullptr; + AstAssign* assignp = nullptr; AstConst* const constp = VN_CAST(nodep, Const); const bool useConstPool = constp // Is a constant && (constp->width() >= STATIC_CONST_MIN_WIDTH) // Large enough @@ -134,7 +119,19 @@ class PremitVisitor final : public VNVisitor { nodep->dtypep()}; m_cfuncp->addInitsp(varp); // Put assignment before the referencing statement - insertBeforeStmt(new AstAssign{fl, new AstVarRef{fl, varp, VAccess::WRITE}, nodep}); + assignp = new AstAssign{fl, new AstVarRef{fl, varp, VAccess::WRITE}, nodep}; + if (m_inWhilep) { + // Statements that are needed for the 'condition' in a while + // actually have to be put before & after the loop, since we + // can't do any statements in a while's (cond). + m_inWhilep->addPrecondsp(assignp); + } else if (m_inTracep) { + m_inTracep->addPrecondsp(assignp); + } else if (m_stmtp) { + m_stmtp->addHereThisAsNext(assignp); + } else { + assignp->v3fatalSrc("No statement insertion point."); + } } // Do not remove VarRefs to this in V3Const @@ -142,6 +139,9 @@ class PremitVisitor final : public VNVisitor { // Replace node with VarRef to new Var relinker.relink(new AstVarRef{fl, varp, VAccess::READ}); + + // Recursive expressions + if (assignp) iterate(assignp); } // VISITORS @@ -357,7 +357,8 @@ class PremitVisitor final : public VNVisitor { iterateChildren(nodep); // Any strings sent to a display must be var of string data type, // to avoid passing a pointer to a temporary. - for (AstNodeExpr* expp = nodep->exprsp(); expp; expp = VN_AS(expp->nextp(), NodeExpr)) { + for (AstNodeExpr *expp = nodep->exprsp(), *nextp; expp; expp = nextp) { + nextp = VN_AS(expp->nextp(), NodeExpr); if (expp->dtypep()->basicp() && expp->dtypep()->basicp()->isString() && !VN_IS(expp, VarRef)) { createDeepTemp(expp, true); diff --git a/src/V3Task.cpp b/src/V3Task.cpp index 9bd96ef03f..fe6e09498e 100644 --- a/src/V3Task.cpp +++ b/src/V3Task.cpp @@ -1391,7 +1391,10 @@ class TaskVisitor final : public VNVisitor { if (debug() >= 9) nodep->dumpTree("- newstmt: "); UASSERT_OBJ(m_insStmtp, nodep, "Function call not underneath a statement"); if (debug() >= 9) newp->dumpTree("- newfunc: "); - m_insStmtp->addHereThisAsNext(newp); + AstBegin* const tempp = new AstBegin{nodep->fileline(), "[EditWrapper]", newp}; + iterateAndNextNull(newp); + m_insStmtp->addHereThisAsNext(tempp->stmtsp()->unlinkFrBackWithNext()); + VL_DO_DANGLING(tempp->deleteTree(), tempp); } // VISITORS @@ -1552,6 +1555,7 @@ class TaskVisitor final : public VNVisitor { } } void visit(AstWhile* nodep) override { + VL_RESTORER(m_insStmtp); // Special, as statements need to be put in different places // Preconditions insert first just before themselves (the normal // rule for other statement types) @@ -1564,23 +1568,21 @@ class TaskVisitor final : public VNVisitor { m_insStmtp = nullptr; // First thing should be new statement iterateAndNextNull(nodep->stmtsp()); iterateAndNextNull(nodep->incsp()); - // Done the loop - m_insStmtp = nullptr; // Next thing should be new statement } void visit(AstNodeFor* nodep) override { // LCOV_EXCL_LINE nodep->v3fatalSrc( "For statements should have been converted to while statements in V3Begin.cpp"); } void visit(AstNodeStmt* nodep) override { + VL_RESTORER(m_insStmtp); m_insStmtp = nodep; iterateChildren(nodep); - m_insStmtp = nullptr; // Next thing should be new statement } void visit(AstStmtExpr* nodep) override { + VL_RESTORER(m_insStmtp); m_insStmtp = nodep; iterateChildren(nodep); if (!nodep->exprp()) VL_DO_DANGLING(nodep->unlinkFrBack()->deleteTree(), nodep); - m_insStmtp = nullptr; // Next thing should be new statement } void visit(AstSenItem* nodep) override { UASSERT_OBJ(!m_inSensesp, nodep, "Senitem under senitem?"); diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 30407d90d6..05d0114ef9 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -2094,16 +2094,20 @@ class WidthVisitor final : public VNVisitor { // We can't skip this step when width()!=0, as creating a AstVar // with non-constant range gets size 1, not size 0. So use didWidth(). if (nodep->didWidth()) return; - if (nodep->doingWidth()) { // Early exit if have circular parameter definition - UASSERT_OBJ(nodep->valuep(), nodep, "circular, but without value"); + nodep->doingWidth(true); + + // Check for recursive initialization + std::function isRecursive = [&](AstNode* np) -> bool { + return np && np->exists([&](AstNodeVarRef* refp) { + return refp->varp() == nodep || isRecursive(refp->varp()->valuep()); + }); + }; + if (isRecursive(nodep->valuep())) { nodep->v3error("Variable's initial value is circular: " << nodep->prettyNameQ()); pushDeletep(nodep->valuep()->unlinkFrBack()); nodep->valuep(new AstConst{nodep->fileline(), AstConst::BitTrue{}}); - nodep->dtypeFrom(nodep->valuep()); - nodep->didWidth(true); - return; } - nodep->doingWidth(true); + // Make sure dtype is sized nodep->dtypep(iterateEditMoveDTypep(nodep, nodep->subDTypep())); UASSERT_OBJ(nodep->dtypep(), nodep, "No dtype determined for var");