[EDIF] Fixes rare bus renaming collision

[clavin-xlnx]

There are two competing issues relevant to this fix.

1. There are unfortunately situations where an EDIFCell can have two EDIFPort objects, a single bit port and a bussed port with the same name, for example my_signal and my_signal[1:0]. We resolve this naming collision by storing the bussed version with a [ suffix, my_signal[.
2. In some designs, it is possible to have two EDIFPort objects belonging to the same EDIFCell such that when one bussed name is EDIF legalized, it collides with another valid EDIFPort name. For example, phy_ctrl[1:0] and phy_ctrl_[13:0]. Due to the possible naming collision issues from issue 1, all bussed ports are given a [ suffix to the name and thus the legal names of busses are all extended by one character, phy_ctrl (original) -> phy_ctrl[ (stored) -> phy_ctrl_ (legalized EDIF name). This leads to ambiguity for the other bused port on the cell and leads to reading problems of the netlist (see test added in the PR to illustrate).

This PR attempts to resolve both of these issue by removing the trailing [ before legalization and also checking to see if there is indeed a colliding single-bit port name that collides with the bussed port name, if there is, it is given an additional suffix during EDIF naming legalization and stored in a separate map, otherwise, the renaming proceeds as normal. This has a small impact to runtime (~5% EDIF write time). This does not impact EDIF read.

This also fixes a connectivity issue when generating multipliers (MultGenerator) where it was blindly connecting all signals as busses. This now detects if it is a bus or single bit port and connects accordingly.

[eddieh-xlnx]

So it looks like the common (non-collision) path now has an extra:

1. EDIFPort.getBusName(/*keepOpenBracket*/ false) (instead of true) which involves constructing a new sub-String that strips any [:

   RapidWright/src/com/xilinx/rapidwright/edif/EDIFPort.java

   Lines 157 to 159 in 8797324

   	if (!keepOpenBracket && busName.charAt(busName.length() - 1) == '[') {
   	return busName.substring(0, busName.length() - 1);
   	}

2. An extra EDIFCell.getPort() hash lookup (or two; see below)

I can see that EDIFCell.getPort() also has some logic with [:

RapidWright/src/com/xilinx/rapidwright/edif/EDIFCell.java

Lines 280 to 288 in 8797324

	public EDIFPort getPort(String name) {
	if (ports == null) return null;
	EDIFPort port = ports.get(name);
	// For callers who have a port name and its unknown if its a bus, attempt a check with adding the '[' suffix
	if (port == null && name.charAt(name.length() - 1) != '[') {
	port = ports.get(name + "[");
	}
	return port;
	}

Am I right in interpreting that 1. will strip the [ suffix, and then 2. will try and lookup with this (unfortunate_name in this test, which will fail) and then it will do a second lookup (with a new string) back on unfortunate_name[ and find a hit)?

[clavin-xlnx]

Am I right in interpreting that 1. will strip the [ suffix, and then 2. will try and lookup with this (unfortunate_name in this test, which will fail) and then it will do a second lookup (with a new string) back on unfortunate_name[ and find a hit)?

That is correct. This is the mechanics used to avoid the root bus name collision problem with single bit ports of the same root name.

To be clear, this PR only modifies how the legalization of the EDIF name on export is handled. The internal mechanics/storage/retrieval remain the same.

[eddieh-xlnx]

That is correct. This is the mechanics used to avoid the root bus name collision problem with single bit ports of the same root name.

To be clear, this PR only modifies how the legalization of the EDIF name on export is handled. The internal mechanics/storage/retrieval remain the same.

My analysis here was to see if we can explain where the 5% performance impact comes from. Could this be reduced now with your latest changes?