check: Un-sigmap the check command

[povik]

The change here modifies the check command not to rely on sigmap, instead considering the connections explicitly. So it catches cases like

module top;
	wire a, b, c;
	assign a = b;
	assign a = c;
endmodule

which are not caught otherwise.

I suspect there are hidden issues with module connections being made in the reverse order. That is, with some passes making the connections in reverse and other passes expecting the RHS of connections to always be the driver.

[povik]

From the Jenkins log:

make[1]: Entering directory '/var/lib/jenkins/workspace/Yosys-PR/tests/techmap'
Warning: multiple conflicting drivers for top.\o:
    assignment of $not$<<EOT:3$1100_Y
    assignment of $abc$1186$o
Warning: multiple conflicting drivers for top.\w:
    assignment of $and$<<EOT:2$1099_Y
    assignment of $abc$1186$w
ERROR: Found 2 problems in 'check -assert'.
make[1]: *** [run-test.mk:6: abc9.ys] Error 1

I guess the suspicion was right?

[whitequark]

IIRC there were some additional issues related to not using sigmap there. Talk to Claire maybe?

[clairexen]

i'm all for adding that functionality to check, but can we add the following two options, instead of changing the default behavior?

    -alias
        Interpret wire-to-wire connections as directionless aliases.
        (This is currently the default behavior.)

    -conn
        Interpret wire-to-wire connections as directional buffers.
        (This may be made the default behavior in the future.)

Note: You can define sigmap as following to turn sigmap() into a no-op when in conn_mode:

    SigMap sigmap(conn_mode ? nullptr : module);

[povik]

So, about the outstanding questions from last dev meeting:

How does SigMap pick a representative of a group of connected wires?

SigMap::add gets called for each connection with from being the connection LHS and to being the RHS.

yosys/kernel/sigtools.h

Lines 265 to 288 in 11b9deb

	void add(const RTLIL::SigSpec& from, const RTLIL::SigSpec& to)
	{
	log_assert(GetSize(from) == GetSize(to));
	for (int i = 0; i < GetSize(from); i++)
	{
	int bfi = database.lookup(from[i]);
	int bti = database.lookup(to[i]);
	const RTLIL::SigBit &bf = database[bfi];
	const RTLIL::SigBit &bt = database[bti];
	if (bf.wire || bt.wire)
	{
	database.imerge(bfi, bti);
	if (bf.wire == nullptr)
	database.ipromote(bfi);
	if (bt.wire == nullptr)
	database.ipromote(bti);
	}
	}
	}

If neither side is constant, database.ipromote doesn't get called, so it's up to the imerge behavior.

yosys/kernel/hashlib.h

Lines 1132 to 1139 in 11b9deb

	void imerge(int i, int j)
	{
	i = ifind(i);
	j = ifind(j);
	if (i != j)
	parents[i] = j;
	}

As I read it the representative of what was formerly the RHS class wins to be the representative of the joint class. Summary: driver of the tree of connected wires, i.e. the wire that is driven by a cell, gets to be the sigmap() representative of a class of connected wires.

Does it even matter if connections are interpreted to be directionless aliases?

As far as I can tell that's not the original RTLIL semantics and existing passes are susceptible to bugs if the interpretation is relaxed. Cases in point:

• Verilog backend dumps connections with assign lhs = rhs
• show draws arrows
• expose in cut or input mode
• insbuf
• iopadmap

[povik]

Another case currently not caught by check (stumbled on by @mattvenn):

module top;
	wire a;
	assign a = 0;
	assign a = 1;
endmodule

With the change here it's caught:

Checking module top...
Warning: multiple conflicting drivers for top.\a:
    assignment of 1'0
    assignment of 1'1
Found and reported 1 problems.