Fix assertion failure in V3Gate

[yTakatsukasa]

verilator/src/V3Gate.cpp

Lines 683 to 684 in 298e0f2

	void recordSubstitution(AstVarScope* vscp, AstNodeExpr* substp, AstNode* logicp) {
	m_hasPending.emplace(logicp, ++m_ord); // It's OK if already present

A registered AstNode in m_hasPending remains even after it is deleted.

verilator/src/V3Gate.cpp

Line 846 in 298e0f2

VL_DO_DANGLING(logicp->unlinkFrBack()->deleteTree(), logicp);

If a newly created AstNode is allocated to the same address as the deleted one, a check in line 693 fails because early exit in line 690 does not work; m_hasPending has the entry for the AstNode though it is for the deleted AstNode.

verilator/src/V3Gate.cpp

Lines 689 to 693 in 298e0f2

	void commitSubstitutions(AstNode* logicp) {
	if (!m_hasPending.erase(logicp)) return; // Had no pending substitutions
	Substitutions& substitutions = m_substitutions(logicp);
	UASSERT_OBJ(!substitutions.empty(), logicp, "No pending substitutions");

I could not make a small test to reproduce this.

[gezalore]

Thanks for tracking this down. The need for this fix causes me concern. If I'm reading this right, the first thing we do in that with the driver of the variable is commit all the pending substitution:

verilator/src/V3Gate.cpp

Lines 761 to 768 in 298e0f2

	// Grab the driving logic
	GateLogicVertex* const lVtxp
	= vVtxp->inEdges().frontp()->fromp()->as<GateLogicVertex>();
	if (!lVtxp->reducible()) continue;
	AstNode* const logicp = lVtxp->nodep();
	// Commit pending optimizations to driving logic, as we will re-analyze
	commitSubstitutions(logicp);

The first thing that does is it removes the pointer from m_pending:

verilator/src/V3Gate.cpp

Lines 689 to 690 in 298e0f2

	void commitSubstitutions(AstNode* logicp) {
	if (!m_hasPending.erase(logicp)) return; // Had no pending substitutions

After that, the only place where it can be added back is here, where we check the sinks (consumers) of the variable:

verilator/src/V3Gate.cpp

Line 825 in 298e0f2

recordSubstitution(vscp, substp, dstVtxp->nodep());

For logicp to end up back in the m_pending map, dstVtxp->nodep() (the sink of the variable), must be the same as logicp.

However, this condition just above, is there precisely to prevent such substitution (where you would substitute the driver logic into itself):

verilator/src/V3Gate.cpp

Lines 810 to 819 in 298e0f2

	// If the consumer logic writes one of the variables that the substitution
	// is reading, then we would get a cycles, so we cannot do that.
	bool canInline = true;
	for (V3GraphEdge& dedge : dstVtxp->outEdges()) {
	const GateVarVertex* const consVVertexp = dedge.top()->as<GateVarVertex>();
	if (readVscps.count(consVVertexp->varScp())) {
	canInline = false;
	break;
	}
	}

Could you check if this is indeed the case (logicp == dstVtxp->nodep()) on line 825. If that is the case, we are missing something in GateOkVisitor which gathers the variables read by the driving logicp.

[yTakatsukasa]

I added
UASSERT_OBJ(logicp != dstVtxp->nodep(), logicp, "unexpected match"); just before recordSubstitution() in line 825, then the assertion failed.

I'll dump and check the AST tomorrow.

[yTakatsukasa]

I added v3Global.rootp()->dumpTree(std::cerr); here and looked how vscp is used.

verilator/src/V3Gate.cpp

Lines 799 to 800 in 298e0f2

	vscp->dumpTree("substituting: ");
	substp->dumpTree(" with: ");

The AstVarScope is not used at all, it is just declared, but not referred.

In _051_split.tree which is just before V3Gate, the AstVarScope is referred both as LV and RV.
I'm not yet friendly enough with V3Gate to guess how that can happen.

[gezalore]

V3Gate has several unique steps (see V3Gate::gateAll), the inlining step (where this crash happens) itself has 2 sub-steps (see GateInline::apply). The graph is built as the first step, at the beginning of V3Gate, and should be adjusted with each edit in later steps, maybe we fail to do that in some cases. If you are able to create a test case you can share that reproduces the same behaviour (UASSERT_OBJ(logicp != dstVtxp->nodep(), logicp, "unexpected match");), I'm also happy to help look into it.