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.

[yTakatsukasa]

Phew! It took so long to understand what's going on and make a simple example to trigger the case.

     inside:  ALWAYS 0x5641c8843a80 {e34ad} u2=0x5641c8858e18 [always_comb]
      inside: 2: ASSIGN 0x5641c88ff960 {e35at} @dt=0x5641c8865a50@(G/w2)
      inside: 2:1: COND 0x5641c88fcb70 {e35av} @dt=0x5641c8865a50@(G/w2)
      inside: 2:1:1: VARREF 0x5641c88fe270 {e35ak} @dt=0x5641c8860c90@(G/w1)  sel [RV] <- VARSCOPE 0x5641c88608a0 {e26ct} u1=0x5641c8840de0 @dt=0x5641c8860c90@(G/w1)  TOP.__PVT__t__DOT__u_bug5101->sel [scopep=0x5641c883ffc0] -> VAR 0x5641c886a660 {e26ct} @dt=0x5641c8860c90@(G/w1)  sel INPUT [VSTATIC]  WIRE
      inside: 2:1:2: VARREF 0x5641c884a360 {e35av} @dt=0x5641c8865a50@(G/w2)  in0 [RV] <- VARSCOPE 0x5641c8842250 {e26bh} u1=0x5641c88627b0 @dt=0x5641c8865a50@(G/w2)  TOP.__PVT__t__DOT__u_bug5101->in0 [scopep=0x5641c883ffc0] -> VAR 0x5641c8869a70 {e26bh} @dt=0x5641c8865a50@(G/w2)  in0 INPUT [VSTATIC]  WIRE
      inside: 2:1:3: EXPRSTMT 0x5641c8865bf0 {e36ar} @dt=0x5641c8865a50@(G/w2)
      inside: 2:1:3:1: COMMENT 0x5641c8841be0 {e36ar}  Function: incr
      inside: 2:1:3:1: ASSIGN 0x5641c88670c0 {e36aw} @dt=0x5641c8865a50@(G/w2)
      inside: 2:1:3:1:1: VARREF 0x5641c8865870 {e36aw} @dt=0x5641c8865a50@(G/w2)  in1 [RV] <- VARSCOPE 0x5641c8844050 {e26cd} u1=0x5641c8869480 @dt=0x5641c8865a50@(G/w2)  TOP.__PVT__t__DOT__u_bug5101->in1 [scopep=0x5641c883ffc0] -> VAR 0x5641c886a2b0 {e26cd} @dt=0x5641c8865a50@(G/w2)  in1 INPUT [VSTATIC]  WIRE
      inside: 2:1:3:1:2: VARREF 0x5641c8841ab0 {e28bq} @dt=0x5641c8865a50@(G/w2)  __Vfunc_incr__0__in [LV] => VARSCOPE 0x5641c884a480 {e28bq} u1=0x5641c885de90 @dt=0x5641c8865a50@(G/w2)  TOP.__PVT__t__DOT__u_bug5101->__Vfunc_incr__0__in [T] [scopep=0x5641c883ffc0] -> VAR 0x5641c8841990 {e28bq} @dt=0x5641c8865a50@(G/w2)  __Vfunc_incr__0__in BLOCKTEMP  
      inside: 2:1:3:1: ASSIGN 0x5641c883ea40 {e30al} @dt=0x5641c8865a50@(G/w2)
      inside: 2:1:3:1:1: ADD 0x5641c88fe580 {e30aq} @dt=0x5641c8865a50@(G/w2)
      inside: 2:1:3:1:1:1: CONST 0x5641c883d710 {e30aq} @dt=0x5641c8865a50@(G/w2)  2'h1
      inside: 2:1:3:1:1:2: VARREF 0x5641c8867f40 {e30an} @dt=0x5641c8865a50@(G/w2)  __Vfunc_incr__0__in [RV] <- VARSCOPE 0x5641c884a480 {e28bq} u1=0x5641c885de90 @dt=0x5641c8865a50@(G/w2)  TOP.__PVT__t__DOT__u_bug5101->__Vfunc_incr__0__in [T] [scopep=0x5641c883ffc0] -> VAR 0x5641c8841990 {e28bq} @dt=0x5641c8865a50@(G/w2)  __Vfunc_incr__0__in BLOCKTEMP
      inside: 2:1:3:1:2: VARREF 0x5641c8867ab0 {e30ah} @dt=0x5641c8865a50@(G/w2)  __PVT__incr__Vstatic__tmp [LV] => VARSCOPE 0x5641c885c810 {e29at} u1=0x5641c8841050 @dt=0x5641c8865a50@(G/w2)  TOP.__PVT__t__DOT__u_bug5101->__PVT__incr__Vstatic__tmp [scopep=0x5641c883ffc0] -> VAR 0x5641c88fdf60 {e29at} @dt=0x5641c8865a50@(G/w2)  __PVT__incr__Vstatic__tmp [VSTATIC]  VAR
      inside: 2:1:3:1: ASSIGN 0x5641c88fe860 {e31ah} @dt=0x5641c8865a50@(G/w2)
      inside: 2:1:3:1:1: VARREF 0x5641c8852600 {e31ao} @dt=0x5641c8865a50@(G/w2)  __PVT__incr__Vstatic__tmp [RV] <- VARSCOPE 0x5641c885c810 {e29at} u1=0x5641c8841050 @dt=0x5641c8865a50@(G/w2)  TOP.__PVT__t__DOT__u_bug5101->__PVT__incr__Vstatic__tmp [scopep=0x5641c883ffc0] -> VAR 0x5641c88fdf60 {e29at} @dt=0x5641c8865a50@(G/w2)  __PVT__incr__Vstatic__tmp [VSTATIC]  VAR
      inside: 2:1:3:1:2: VARREF 0x5641c88420c0 {e31ah} @dt=0x5641c8865a50@(G/w2)  __Vfunc_incr__0__Vfuncout [LV] => VARSCOPE 0x5641c88fe490 {e28az} u1=0x5641c883ec50 @dt=0x5641c8865a50@(G/w2)  TOP.__PVT__t__DOT__u_bug5101->__Vfunc_incr__0__Vfuncout [T] [scopep=0x5641c883ffc0] -> VAR 0x5641c88fd8c0 {e28az} @dt=0x5641c8865a50@(G/w2)  __Vfunc_incr__0__Vfuncout BLOCKTEMP
      inside: 2:1:3:2: VARREF 0x5641c885ddc0 {e36ar} @dt=0x5641c8865a50@(G/w2)  __Vfunc_incr__0__Vfuncout [RV] <- VARSCOPE 0x5641c88fe490 {e28az} u1=0x5641c883ec50 @dt=0x5641c8865a50@(G/w2)  TOP.__PVT__t__DOT__u_bug5101->__Vfunc_incr__0__Vfuncout [T] [scopep=0x5641c883ffc0] -> VAR 0x5641c88fd8c0 {e28az} @dt=0x5641c8865a50@(G/w2)  __Vfunc_incr__0__Vfuncout BLOCKTEMP
      inside: 2:2: VARREF 0x5641c88683d0 {e35ap} @dt=0x5641c8865a50@(G/w2)  out [LV] => VARSCOPE 0x5641c885cec0 {e26dr} u1=0x5641c8850b30 @dt=0x5641c8865a50@(G/w2)  TOP.__PVT__t__DOT__u_bug5101->out [scopep=0x5641c883ffc0] -> VAR 0x5641c88fc9f0 {e26dr} @dt=0x5641c8865a50@(G/w2)  out OUTPUT PORT

AST around always_comb looks like above.

After sel is substituted by 1'b1, the ALWAYS becomes just always_comb out = in0;
EXPRSTMT is totally removed, but TOP.__PVT__t__DOT__u_bug5101->__Vfunc_incr__0__in is still on the Graph.

When V3Gate tries to substitute it, graph thinks the VarScope is defined and referred in the EXPRSTMT, logicpp == dstVtxp.

There could be multiple solutions. I'm not sure which one is better.
a) call commitSubstitutions() more frequently
b) accept logicp == dstVtxp->nodep()and just remove from m_hasPending as in the initial commit
c) improve GateOkVisitor to cover this case
d) .. perhaps more