Duplicate EDIFPortInsts - Expected behaviour?

[jakobwenzel]

Hi,

after hours of debugging, I found out that 131331d subtly changed behaviour when there are duplicate EDIFPortInsts.

Let's use this minimal example:

@Test
    public void test() {

        String designName = "design";
        final EDIFNetlist netlist = EDIFTools.createNewNetlist(designName);

        EDIFCell ec = new EDIFCell(netlist.getWorkLibrary(), "foo");
        EDIFPort port = ec.createPort("in", EDIFDirection.INPUT, 1);


        EDIFCell top = netlist.getTopCell();
        EDIFCellInst cell = ec.createCellInst("a", top);

        EDIFNet netA = top.createNet("netA");
        EDIFNet netB = top.createNet("netB");


        netA.createPortInst(port, cell);
        netA.removePortInst(netA.getPortInsts().iterator().next());

        netB.createPortInst(port, cell);
        EDIFPortInst epi = cell.getPortInsts().iterator().next();


        Assertions.assertEquals(netB, epi.getNet());
    }

We create a cell inst and connect it to one net. Then we change our mind, disconnect and connect to a different net.
When querying the cell inst for which net its port inst connects to, we should get netB, right? Well, before 131331d we did. Now, we get null.

The reason for this change is subtle: Previously, EDIFCellInst.portInsts was a Map. When adding a duplicate entry in EDIFCellInst.addPortInst, the previous one was discarded and the new one was saved in the map. In that change EDIFPortInstList was introduced, which is now used in EDIFCellInst. In case of duplicates, EDIFPortInstList keeps the old value and discards the new one.

This change was unexpected for me and very hard to find. I believe that mirroring the previous behaviour more closely is more intuitive.

[jakobwenzel]

Mirroring the previous behaviour would probably be to overwrite previously stored values with new duplicates here:

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

Lines 42 to 45 in a959449

	// Do not allow duplicates
	if (insertionPoint >= 0) {
	return false;
	}

[eddieh-xlnx]

Fixed by #571.