Closed acomodi closed 3 years ago
gatecat
commented
3 years ago I don't think this configuration is valid, so there is a bug in nextpnr-fpga_interchange's LUT and pseudo pip logic.
I5 (i.e. A6 in this context) is used to select O6 between O5 (the lower half of the LUT function) and the upper half of the LUT function. As O5 is defined to be A5, O6 can't always be A6.
gatecat
commented
3 years ago Looking at the DCP and nextpnr side code, I think the problem is that LUT function constraints are only being considered for LUT route-through pseudo-tile-PIPs and not LUT routethrough site-PIPs. This is something I can look into fixing, if you want.
acomodi
commented
3 years ago All right, I think I might look into this so that I start to get more familiarity with it and nextpnr in general.
I don't think this configuration is valid, so there is a bug in nextpnr-fpga_interchange's LUT and pseudo pip logic.
So, If I understand correctly, we cannot have ever two LUT thrus where one of the pins used is A6, as this is actually a selection pin, which eventually can conflict with the lower half which controls the 05 output. In general though, having two LUT-thrus on pins other than A6 should be fine (provided that A6 is routed to VCC).
acomodi
commented
3 years ago After debugging this problem in more depth in nextpnr, I think what is happening is the following:
A5 to O5 output has been bound to BLUT5.A5 -> O5 for BLUT5) is active and binds the B6 -> B pseudo PIP, which should really not be allowed.I think the fix should be added to the update site routine, to be able to identify also the site-thru PIPs and, based on their activation state, remove from the allowed pseudo PIPs all those which can generate conflicts later on. @gatecat does this sound good?
gatecat
commented
3 years ago One option would be to create wire LUTs to the route-throughs, similar to how pseudo-pips work, which would then mean the usual LUT-blocking code would block the pseudo-pips:
however, it is worth noting that site updates are already a performance critical path and this will only slow things down more. But long term (i.e. on the ~3-month timeframe) I think a substantial performance-focused rewrite of the site related code will be needed in any case.
GitHub
nextpnr portable FPGA place and route tool. Contribute to gatecat/nextpnr development by creating an account on GitHub.
acomodi
commented
3 years ago All right, so, the problem I am having at the moment is how to get the information on how the "other" LUT's connectivity. In other words, all tile pseudo-pips, as far as I have seen, only imply A6 -> O6 connections in the LUT6 bel, therefore I am not sure how to get the info about the LUT5 bel that may contain a LUT-thru.
The solution might be that, if a LUT-thru is present in the LUT5 (or in general in one of the "other" LUT), the pseudo-pip is not valid, or actually, as you suggested, to add the LUT-wire equation to the LUT6 and have the lut-blocking code do the rest.
On a side note, I think that this issue might need to be pushed higher in the priorities, so that we could better test these kinds of situations (unsure if we can control general routing during testing, so to use also tile pseudo pips). The problem is that this specific issue is hard to reproduce in small designs, as the router will not use pseudo pips if it does not need to.
acomodi
commented
3 years ago While further debugging this issue, I came across a similar one, which do not exactly fall in the exact same category though.
In this failing implementation (includes dcp, phys.yaml and netlist.yaml), the same BLUT (SLICE_X5Y39/B6LUT) uses the same A3 pin, both for an explicit LUT and for a site-thru LUT, which I suppose shouldn't be allowed.
gatecat
commented
3 years ago In this failing implementation (includes dcp, phys.yaml and netlist.yaml), the same BLUT (SLICE_X5Y39/B6LUT) uses the same A3 pin, both for an explicit LUT and for a site-thru LUT, which I suppose shouldn't be allowed.
Hmm, as the net appears to be the same for both uses of the pin (LUT and route-thru), and everything else is legal (A6 not used and no apparent output overlap) it should technically be a valid usage - no different to packing two LUT5s sharing some inputs together, which is a valid and intended use of the structure. I'm not immediately sure why Vivado rejects this pattern, there might be some subtlety with route-thrus that means this isn't allowed, or it's a red herring and the problem is elsewhere...
acomodi
commented
3 years ago @gatecat You are right, I missed that the net corresponding to the A3 pin is actually the same, and indeed this might not be the root cause of this issue.
What I see with the report_route_status is the following:
open_checkpoint: Time (s): cpu = 00:00:22 ; elapsed = 00:00:08 . Memory (MB): peak = 6411.957 ; gain = 498.770 ; free physical = 6141 ; free virtual = 12562
report_route_status
Design Route Status
: # nets :
------------------------------------------- : ----------- :
# of logical nets.......................... : 13459 :
# of nets not needing routing.......... : 8841 :
# of internally routed nets........ : 8728 :
# of nets with no loads............ : 113 :
# of routable nets..................... : 4618 :
# of fully routed nets............. : 4617 :
# of nets with routing errors.......... : 1 :
# of nets with antennas/islands.... : 1 :
------------------------------------------- : ----------- :
Nets with Routing Errors:
murax._zz_104
Antenna Nodes: CLBLL_L_X4Y39/CLBLL_L_B
Where the antenna node corresponds to the wire-through. This is quite interesting and the problem might be that the same net is being output from the same site twice, which might not be accepted? Quite unclear at the moment. @clavin-xlnx Do you have some additional insights on what might be wrong here?
clavin-xlnx
commented
3 years ago @acomodi I am looking into the issue. I currently can't get python-fpga-interchange to compile, so if you happen to have the binary Interchange files handy, that might be useful.
acomodi
commented
3 years ago so if you happen to have the binary Interchange files handy, that might be useful.
@clavin-xlnx Sure, I have packed the physical and logical capnp netlists alongside the dcp and the yamls: failing_murax.tar.gz
clavin-xlnx
commented
3 years ago One thing I am not sure about is that there is a psuedo PIP CLBLL_L_X4Y39/CLBLL_L.CLBLL_L_B3->>CLBLL_L_B that makes the route thru connection intended, but I don't see it in the routing of the net murax._zz_104. I've looked through >100 other designs on Series 7 to try and find a similar example, but none seem to use any psuedo PIPs like one above. What is more surprising is that using Vivado's own find_routing_path:
find_routing_path -from [get_nodes CLBLL_L_X4Y39/CLBLL_L_B3] -to [get_nodes CLBLL_L_X4Y39/CLBLL_L_B]
No path foundIt doesn't seem like Vivado generally produces routes that take advantage of that PIP, but it may still be valid. I just have to keep digging a little deeper.
clavin-xlnx
commented
3 years ago Strangely, if I move backwards by one hop, the method does find a path from B3->B:
find_routing_path -from [get_nodes INT_L_X4Y39/IMUX_L17] -to [get_nodes CLBLL_L_X4Y39/CLBLL_LL_B]
INT_L_X4Y39/IMUX_L17 CLBLL_L_X4Y39/CLBLL_LL_B3 CLBLL_L_X4Y39/CLBLL_LL_BThe main problem I see is that this kind of pseudo PIP is used quite often in the linked design, and in all the other examples I found, all the nets were correctly routed. One of the main differences between a correctly routed net using a LUT-thru and the antenna net was that, in the latter, the same input net to B3 is used both as a "normal" LUT5 input and also as a LUT6 route-thru.
gatecat
commented
3 years ago fwiw, this TCL - run on an empty xc7a35t design - seems to demonstrate successful use of the pip shared with a LUT (ignoring the unrouted Vcc, but that should be immaterial here):
# Clear out the design
route_design -unroute
remove_cell *
remove_net *
create_cell -reference VCC vcc_src
create_net vcc_net
connect_net -net vcc_net -objects vcc_src
create_cell -reference LUT1 driver_lut_1
place_cell driver_lut_1 SLICE_X4Y39/B6LUT
create_cell -reference LUT4 sink_lut_1
place_cell sink_lut_1 SLICE_X5Y39/B5LUT
create_net output_net
connect_net -net output_net -objects sink_lut_1/O
# Force use of the B3 pin with some dummy pins
connect_net -net vcc_net -objects [list sink_lut_1/I1 sink_lut_1/I2]
# Route a net from the driver LUT to the B3 pin of the sink LUT,
# and also routed through the B3 pin back to the driver LUT
create_net b3_net
connect_net -net b3_net -objects [list driver_lut_1/O sink_lut_1/I0 driver_lut_1/I0]
set_property ROUTE {CLBLL_L_X4Y39/CLBLL_LL_B CLBLL_L_X4Y39/CLBLL_LOGIC_OUTS13 \
INT_L_X4Y39/NW2BEG1 INT_R_X3Y40/EL1BEG0 INT_L_X4Y40/SL1BEG0 INT_L_X4Y39/IMUX_L16 \
CLBLL_L_X4Y39/CLBLL_L_B3 \
CLBLL_L_X4Y39/CLBLL_L_B CLBLL_L_X4Y39/CLBLL_LOGIC_OUTS9 INT_L_X4Y39/BYP_ALT4 \
INT_L_X4Y39/BYP_BOUNCE4 INT_L_X4Y39/IMUX_L12 CLBLL_L_X4Y39/CLBLL_LL_B6} [get_nets b3_net]
gatecat
commented
3 years ago This TCL aims to reproduce the "two output sitepins on the same net" hypothesis that was also discussed, and also it seems like the route-thru works fine:
# Clear out the design
route_design -unroute
remove_cell *
remove_net *
# Create a DFF with D driven by Q for testing
create_cell -reference FDRE test_dff
create_net test_net
connect_net -net test_net -objects [list test_dff/D test_dff/Q]
place_cell test_dff SLICE_X5Y39/AFF
# Route from DFF Q to D, but via LUT route-thru in the same site
# FF output -> B3->B LUT routethrough -> AX
set_property ROUTE {CLBLL_L_X4Y39/CLBLL_L_AQ CLBLL_L_X4Y39/CLBLL_LOGIC_OUTS0 INT_L_X4Y39/IMUX_L16 CLBLL_L_X4Y39/CLBLL_L_B3 \
CLBLL_L_X4Y39/CLBLL_L_B CLBLL_L_X4Y39/CLBLL_LOGIC_OUTS9 INT_L_X4Y39/FAN_ALT2 INT_L_X4Y39/FAN_BOUNCE2 \
INT_L_X4Y39/BYP_ALT0 INT_L_X4Y39/BYP_L0 CLBLL_L_X4Y39/CLBLL_L_AX} [get_nets test_net]
So it seems like there might be some very complex interaction going on here...
acomodi
commented
3 years ago Yeah, I have experimented a bit more and it seems that both situations are actually accepted when dealt by Vivado. The following TCL should enclose both of them and mimic the failing antenna net:
open_checkpoint test.dcp
# Clear out the design
route_design -unroute
remove_cell *
remove_net *
create_cell -reference LUT2 driver_lut_1
place_cell driver_lut_1 SLICE_X4Y39/B6LUT
create_cell -reference LUT1 sink_lut_1
place_cell sink_lut_1 SLICE_X5Y39/B5LUT
create_cell -reference FDRE fdre_1
place_cell fdre_1 SLICE_X5Y39/AFF
create_net test_net
connect_net -net test_net -objects [list sink_lut_1/O driver_lut_1/I0]
route_design
# Route a net from the driver LUT to the B3 pin of the sink LUT,
# and also routed through the B3 pin back to the driver LUT
create_net b3_net
connect_net -net b3_net -objects [list fdre_1/Q sink_lut_1/I0 driver_lut_1/I1]
reset_property LOCK_PINS [get_cells sink_lut_1]
set_property LOCK_PINS {I0:A3} [get_cells sink_lut_1]
route_design -unroute -nets [get_nets b3_net]
set_property ROUTE {CLBLL_L_X4Y39/CLBLL_L_AQ \
CLBLL_L_X4Y39/CLBLL_LOGIC_OUTS0 \
INT_L_X4Y39/IMUX_L16 \
CLBLL_L_X4Y39/CLBLL_L_B3 \
CLBLL_L_X4Y39/CLBLL_L_B CLBLL_L_X4Y39/CLBLL_LOGIC_OUTS9 INT_L_X4Y39/BYP_ALT4 \
INT_L_X4Y39/BYP_BOUNCE4 INT_L_X4Y39/IMUX_L12 CLBLL_L_X4Y39/CLBLL_LL_B6} [get_nets b3_net]
write_checkpoint -force test3.dcp
I wonder whether the DCP generated starting from the interchange might be missing some information required by Vivado to handle this case?
gatecat
commented
3 years ago It seems like I can 'fix' the broken design by manually patching the ROUTE property of the offending net using
set_property ROUTE "( { CLBLL_L_X4Y39/CLBLL_L_AQ CLBLL_LOGIC_OUTS0 { IMUX_L16 CLBLL_L_B3 CLBLL_L_B CLBLL_LOGIC_OUTS9 NL1BEG0 IMUX_L15 CLBLL_LL_B1} IMUX_L8 CLBLL_LL_A5 CLBLL_LL_A CLBLL_LL_AMUX CLBLL_LOGIC_OUTS20 { IMUX_L36 CLBLL_L_D2 } IMUX_L20 CLBLL_L_C2 } ) " [get_nets murax._zz_104]in the broken form it is
( { CLBLL_L_X4Y39/CLBLL_L_B CLBLL_LOGIC_OUTS9 NL1BEG0 IMUX_L15 CLBLL_LL_B1 } ) ( { CLBLL_L_X4Y39/CLBLL_L_AQ CLBLL_LOGIC_OUTS0 { IMUX_L16 CLBLL_L_B3 } IMUX_L8 CLBLL_LL_A5 CLBLL_LL_A CLBLL_LL_AMUX CLBLL_LOGIC_OUTS20 { IMUX_L36 CLBLL_L_D2 } IMUX_L20 CLBLL_L_C2 } )as if the connectivity across the PIP isn't being seen.
Looking at the YAML, it seems like the route-thru is specified as a bel and so the PIP is never actually being passed to Vivado?
- routeSegment:
pip:
tile: INT_L_X4Y39
wire0: LOGIC_OUTS_L0
wire1: IMUX_L16
forward: true
isFixed: false
branches:
- routeSegment:
pip:
tile: CLBLL_L_X4Y39
wire0: CLBLL_IMUX16
wire1: CLBLL_L_B3
forward: true
isFixed: false
branches:
- routeSegment:
sitePin:
site: SLICE_X5Y39
pin: B3
branches:
- routeSegment:
belPin:
site: SLICE_X5Y39
bel: B3
pin: B3
branches:
- routeSegment:
belPin:
site: SLICE_X5Y39
bel: B6LUT
pin: A3
branches:
- routeSegment:
belPin:
site: SLICE_X5Y39
bel: B6LUT
pin: O6
branches:
- routeSegment:
sitePIP:
site: SLICE_X5Y39
bel: BUSED
pin: 0
isFixed: false
isInverting: false
branches:
- routeSegment:
belPin:
site: SLICE_X5Y39
bel: B
pin: B
branches:
- routeSegment:
sitePin:
site: SLICE_X5Y39
pin: B
branches:
- routeSegment:
pip:
tile: CLBLL_L_X4Y39
wire0: CLBLL_L_B
wire1: CLBLL_LOGIC_OUTS9
forward: true
isFixed: false
branches:
- routeSegment:
pip:
tile: INT_L_X4Y39
wire0: LOGIC_OUTS_L9
wire1: NL1BEG0
forward: true
isFixed: false
branches:
- routeSegment:
pip:
tile: INT_L_X4Y39
wire0: NL1END_S3_0
wire1: IMUX_L15
forward: true
isFixed: falseIt seems like this might be better if it just included the route-thru PIP as a regular PIP and not all the site routing. Some more investigation needed to find out what is happening here compared to what is supposed to happen, after all LUT route-thrus are working fine 99% of the time.
gatecat
commented
3 years ago Even stranger there are other cases where - correctly behaving - LUT route-thru PIPs are written as if they were regular PIPs in the YAML:
- routeSegment:
pip:
tile: CLBLM_L_X10Y60
wire0: CLBLM_IMUX6
wire1: CLBLM_L_A1
forward: true
isFixed: false
branches:
- routeSegment:
pip:
tile: CLBLM_L_X10Y60
wire0: CLBLM_L_A1
wire1: CLBLM_L_A
forward: true
isFixed: false
branches:
- routeSegment:
pip:
tile: CLBLM_L_X10Y60
wire0: CLBLM_L_A
wire1: CLBLM_LOGIC_OUTS8
forward: true
isFixed: falseSo something must be up with this specific PIP that's causing it to be written as a bel rather than a PIP?
acomodi
commented
3 years ago So, afaiu these are actually two different route-thrus that happen at two different stages.
gatecat
commented
3 years ago Sorry for somewhat thinking aloud here (edit and I think our messages collided somewhat...)
I think the problem is that the LUT route-thru site pip, rather than tile pip is being used. It is unclear in the current semantics of the interchange format if this is legal in this case or not. If it is then some tweaks in the DCP generation will be needed to get Vivado to recognise the site pip.
Otherwise, if it's not legal, this nextpnr check needs to be fixed:
to specifically reject the use of a site PIP when the site has been left and then re-entered; rather than with the simpler constraint that the driver of the net is in the same site as the site PIP.
nextpnr portable FPGA place and route tool. Contribute to YosysHQ/nextpnr development by creating an account on GitHub.
acomodi
commented
3 years ago Sorry for somewhat thinking aloud here
No probs, I am all for thinking aloud, so it is easier to get to the bottom of the problem.
Now I see what you mean, and I think I agree, as in, a net going through a site PIP should not be able to "escape" the site and this indeed needs to be marked as illegal. A LUT-thru site PIP should be allowed if the sink is in the same path and in the same site where the LUT-thru is present, otherwise, as you pointed out, the site PIP should really be a tile PIP and handled by the router. And this should extend to all site-thrus of course and not only LUT ones.
clavin-xlnx
commented
3 years ago Ok, I have confirmed on my side when a route is passing through a site by the means of an input site pin to an output site pin, it must use the Tile PIP (in this case CLBLL_L_X4Y39/CLBLL_L.CLBLL_L_B3->>CLBLL_L_B), instead of site routing (intra-site routing resources can't generally be used for inter-site routing). I tried using @gatecat's Tcl fix, but I was still getting issues when running report_route_status, but I think the spirit of the fix is the same. I was able to confirm by making the switch in RapidWright by reading in the Interchange files, switching from site routing to a Tile PIP and then writing to a DCP. This yields 0 errors when running report_route_status and the net and design appears to be fully routed in Vivado. Here is the RapidWright code that swaps the routing (from intra-site to inter-site):
Design design = PhysNetlistReader.readPhysNetlist("murax_basys3.phys",
LogNetlistReader.readLogNetlist("murax_basys3.netlist"));
// Add Tile route thru PIP
design.getNet("murax._zz_104").addPIP(
new PIP("CLBLL_L_X4Y39/CLBLL_L.CLBLL_L_B3->>CLBLL_L_B", design.getDevice()));
// Remove existing site routing
SiteInst si = design.getSiteInst("SLICE_X5Y39");
si.unrouteIntraSiteNet(si.getBEL("B6LUT").getPin("O6"), si.getBEL("B").getPin("B"));
design.writeCheckpoint("murax_basys3_fixed.dcp");As discussed, this opens the question of should such a preference be part of the Interchange, or, should we just clarify what should happen in intra-site vs inter site routing. From my perspective, it seems the issue here is that an intra-site routing solution was used to solve an inter-site routing problem. In Vivado, routing inside of a site happens prior to inter-site routing and uses intra-site routing resources (SitePIPs and SiteWire net annotations) to route inside a site. Tile route thru PIPs are present to allow inter-site routing to make use of the site resources. Generally, when performing inter-site routing, all the sources and sinks are routed out to a site pin already, so there should generally be no need (all placements remaining static) to route inside a site.
For the Interchange, nextpnr and other tools, I assumed there is a distinction/ordering between intra-site routing and inter-site routing. However, the nature of how they are being executed is perhaps more broad than that. My personal assumption was that intra-site routing happens prior to inter-site routing just as I assume placement takes place prior to routing. Perhaps @gatecat can add more perspective here.
acomodi
commented
3 years ago Thanks for double-checking this @clavin-xlnx. At this point, I think we have a pretty good understanding of what is happening, and I believe that a fix should be added to the site router, which is the one that yields intra-site routes. The particular faulty path should be generated at that stage, which is prior to the general router, which is disallowed to perform intra-site routing and can only perform inter-site routing.
As far as I have seen, at the moment, the site router is aware of the immediate adjacent routing resources, which might allow for this situation to happen (e.g. in this case, a net is allowed to be routed through a neighbouring SLICE using site resources, entering and exiting the site without being produced or consumed by any source/sink). Therefore, another possible location for a fix is in the site router indeed.
acomodi
commented
3 years ago With https://github.com/YosysHQ/nextpnr/pull/700 and https://github.com/YosysHQ/nextpnr/pull/695, the illegal site-thru should now be solved.
What remains is the original issue for which a tile pseudo PIP is used illegally, based on the LUT5 status. That is, if the tile pseudo PIP passes through the A6 -> O6 pins of the LUT6, the LUT5 cannot have a site-thru as well.
This is a case that cannot be generalized IMO, unless we assume that all LUTs work in the same way (e.g. the uppermost input is used as a mux selector). Probably the right way to handle this is to use the constraint mechanism (which I think still needs to be implemented in nextpnr), so that some PIPs get excluded for specific situations.
As a very naive workaround, if there is no other immediate solution, I think that we can temporarily disable pseudo PIPs passing through a LUT6. @gatecat thoughts on that?
gatecat
commented
3 years ago Tile pseudo PIPs should be checking the LUT equation already:
I think the problem is that if a site LUT route-through is used (for example to access a FF, which is still legal post recent patches), a wire LUT isn't created so the pseudo PIP validity checker doesn't see that A6 is unavailable.
nextpnr portable FPGA place and route tool. Contribute to YosysHQ/nextpnr development by creating an account on GitHub.
acomodi
commented
3 years ago Ok, I think I get it and have a possible solution for this. As you said, the LUT route thru at site level are not marked as such.
I believe that yet another piece of data needs to be recorded for the site status, which stores all LUT BELs used as site pseudo PIP, as well as the input/output pins, such that the wire lut can be added as a cell in the lut mapper, which will then have a complete information on the lut element, discarding invalid tile psuedo PIPs.
gatecat
commented
3 years ago My understanding is that the current site router implies this when the bel associated with the site PIP is a LUT bel. But adding explicit tags for this wouldn't be a bad idea, if you do look into this also consider SymbiFlow/fpga-interchange-schema#47 at the same time.
acomodi
commented
3 years ago My bad, the solution I was thinking of does not imply modifying the schema, but nextpnr, so that when the site router has finished, we have a lookup to check which LUT wires (if any) were used in each site.
gatecat
commented
3 years ago Yes, that sounds good
While debugging the Murax design with the interchange format, the following DRC errors were generated:
These correspond to LUTs where both the O5 and O6 pins were used to have signals routed through the LUTs (one as a pseudo PIP and the other as a site PIP).
The LUT equations in this case are:
Given that this should be a valid configuration, I wonder whether the issue might be happening when writing the DCP with RapidWright.
In addition, the following warnings appear as well, which I believe should be taken into consideration: