Xilinx / RapidWright

Build Customized FPGA Implementations for Vivado
http://www.rapidwright.io
Other
286 stars 109 forks source link

RW links net to the wrong port in RAM32M DI* bus signal #749

Closed reillymck closed 11 months ago

reillymck commented 1 year ago

I have attached some sample code/dcp to reproduce this error. I am using Vivado 2023.1

bad_port_py.txt design.zip design_edf.txt

What has happened, is the ADDR and DO bus signals transfered correctly, but the DI bus signals have the wires swapped. Here is some sample prints of running my transfer_bus function for one of the DI buses: (For your convenience, I have edited my transfer_bus function to make these prints when the cell correlates to the A6LUT and the transfer is the D signal to the DI[0] pin)

logical_pin = "D"
dest_bus = "DIA"
# Transfer the D input to the ram32m DI0[0] pin
old_edif_cell_inst = edif_cells[0]
new_edif_cell_inst = ram32m
bus = new_edif_cell_inst.getPort(dest_bus)
print(bus.getName())
# 'DIA[1:0]'
old_port = old_edif_cell_inst.getPortInst(logical_pin)
logical_net = old_port.getNet()
idx = bus.getPortIndexFromNameIndex(0)
print(idx)
# 1
new_port = bus.getPortInstNameFromPort(idx)
print(new_port)
# DIA[0]
# Notice how the new port printed in the net is DIA[0]... in the exported edf file/running write_verilog on the new checkpoint shows
# the new port connected to DIA[1].
logical_net.createPortInst(new_port, new_edif_cell_inst)
logical_net.removePortInst(old_port)
for pi in logical_net.getPortInsts():
    print(pi)
'''
PC[4]_i_1/I3
PC[5]_i_2/I0
PC[8]_i_2/I5
PC_reg[4]/Q
inst_addr_OBUF[4]_inst/I
new_ram32m/DIA[0]
'''

RAM32M instance from write_verilog ran after opening the modified checkpoint:

RAM32M #(
    .INIT_A(64'h0000000000000000),
    .INIT_B(64'h0000000000000000),
    .INIT_C(64'h0000000000000000),
    .INIT_D(64'h0000000000000000)) 
    new_ram32m
       (.ADDRA({GND_2,\LIFOi/addr_reg }),
        .ADDRB({GND_2,\LIFOi/addr_reg }),
        .ADDRC({GND_2,\LIFOi/addr_reg }),
        .ADDRD({GND_2,\LIFOi/addr_reg }),
        .DIA({GND_4,inst_addr_OBUF[4]}),
        .DIB({GND_4,inst_addr_OBUF[5]}),
        .DIC({GND_4,inst_addr_OBUF[6]}),
        .DID({GND_4,inst_addr_OBUF[3]}),
        .DOA(dout[4]),
        .DOB(dout[5]),
        .DOC(dout[6]),
        .DOD(dout[3]),
        .WCLK(clk_IBUF_BUFG),
        .WE(wr_en_IBUF));

The EDIF file is also incorrect for the DI* ports. Here is the net that should be connected to DOC[0] pin:

(net (rename inst_addr_OBUF_4_ "inst_addr_OBUF[4]") (joined
          (portref I3 (instanceref PC_4__i_1))
          (portref I0 (instanceref PC_5__i_2))
          (portref I5 (instanceref PC_8__i_2))
          (portref Q (instanceref PC_reg_4_))
          (portref I (instanceref inst_addr_OBUF_4__inst))
          (portref (member DIA_ 1) (instanceref new_ram32m))
          )

Interestingly enough, the new edif file shows the DI ports connected to bit 1 (no mention of bit zero), and the new edif file also shows the DO ports connected to bit 1 (no mention of bit zero), but the verilog instance adds a gnd signal to bit 0 of DI, and then from my understanding shows the DO bus as connected to a single wire. Usually I don't specifically wire the gnd ports and it seems to fill it in for me...

clavin-xlnx commented 1 year ago

I'm not sure I completely understand the task attempted here. One thing to note is that primitives and macro definitions cannot be changed. In the 7 Series, RAM32X1S and RAM32M are macros which are composed of RAMS32 and RAMD32 primitives. In the logical netlist written out as EDIF or Verilog, these macros are turned into primitives (collapsed) and expanded when read into Vivado or RapidWright. When loading a DCP into Vivado, you'll see an INFO message such as:

INFO: [Project 1-111] Unisim Transformation Summary:
  A total of 8 instances were transformed.
  RAM16X1S => RAM32X1S (RAMS32): 7 instances
  RAM32M => RAM32M (RAMD32(x6), RAMS32(x2)): 1 instance 

If you want to make changes, you'll need to make them external to the macros or if you are trying to convert from one to another, you'll need to make sure everything is consistent and compatible external to the macros.

reillymck commented 1 year ago

I do think I am making them external to the macros? That is why I go up two levels in hierarchy -> I change the 4 RAM32X1S that is wrapping the RAMS32 on a SLICEM into a RAM32M. The verilog/edif file that is output then has my RAM32M primitive there. The problem is that for some reason the net is being connected to the port like I expect. The RAM32M has DIA, DIB, DIC, DID bus ports. I want to connect the net on the "D" port of the RAM32X1S on the ALUT to the DIA[0] port on the RAM32M. So, I execute the code snippet that I highlighted above to transfer the ports. And in rapidwright, when I print out the port instances on the net after removing the "D" RAM32X1S port and adding the DIA[0] on RAM32M, it shows this:

PC[4]_i_1/I3
PC[5]_i_2/I0
PC[8]_i_2/I5
PC_reg[4]/Q
inst_addr_OBUF[4]_inst/I
new_ram32m/DIA[0]

So notice that it is showing that this net is connected to my newram32m on port DIA[0]. The bug which I think I have encountered is when I run export edif in rapidwright directly after making this net change, the new edif file shows `(portref (member DIA 1) (instanceref new_ram32m))`, which to my understanding means that the net is connected to DIA[1]. So, I think there is some discrepancy going on here. Shouldn't the edif print for the net match the rapidwright getPortInsts() call for the net? Also, when I load the dcp into Vivado and then run write_verilog, my new_ram32m instance is there, but again, it is showing the net connected to DIA[1] instead of DIA[0], and it has added a GND net to DIA[0] (which I think is just what happens when you have no connection there?).

clavin-xlnx commented 1 year ago

Perhaps the DCP you attached is not correct? It doesn't seem to match with what you are explaining.

reillymck commented 1 year ago

Sorry I do plan on clarifying this issue soon.

reillymck commented 1 year ago

The dcp is for before running bad_port_py.txt. Here is the resulting dcp (unzip for dcp and edf file): modified.zip As you will see in this edf file, the ports are wired to the "1" bit of the DI bus signal instead of the "0" bit.

clavin-xlnx commented 1 year ago

Sorry for the delay. It took a while to understand what you are trying to do and it wasn't clear from the explanation for me to see what your concern is. If I can try to re-explain the design transformation you are attempting, it is this:

In the original design (design.zip which is really design.dcp), there are four primitive macro cell instances of type RAM32X1S:

  1. LIFOi/ram_reg_0_15_3_3
  2. LIFOi/ram_reg_0_15_4_4
  3. LIFOi/ram_reg_0_15_5_5
  4. LIFOi/ram_reg_0_15_6_6

Here is the schematic view of these 4 instances:

image

The task is to delete these 4 instances, create a new instance new_ram32m of type RAM32M and connect it up to the 4 RAM32X1S inputs/outputs such that it 'replaces' them, with the connectivity ending up as:

image

The concern, if I understand correctly, is that RapidWright is not connecting up the DI* and DO* bussed ports as expected (however the ADDR* ports are connected correctly).

It is my understanding the the connections are as expected, but the interpretation of the EDIF and Verilog are misread. To confirm they are connected correctly, it sounds like the desired connectivity for new_ram32m/DIA[0] should be PC_reg[4]/Q and we can confirm this in Vivado:

image

By selecting the bussed port DIA[1:0] on new_ram32m, I get the "Bus Pin Properties" window which shows the desired indexing is correct.

I believe the misunderstanding is that in EDIF, the connection (portref (member DIA_ 1) (instanceref new_ram32m)) is actually referring to bit 0 of the port because the port is defined as 1 downto 0, or the bit array is [1, 0], where the 0th indexed is bit index 1 and the 1st bit index bit is actually bit 0. The Verilog example also follows similarly.

So overall, the connectivity is correct, however, there are a number of critical warnings when I open the modified DCP. I'm not sure if you are trying to preserve the same placement and routing, but these details are not consistent with the changes being made. Although the logical cells were removed, the site routing and placement were not and result in a number of issues.

reillymck commented 11 months ago

Sorry, other third-party tools lead me to believe I should be expecting different output. Thanks for investigating.