interchange: fix site-thru pip legality

[acomodi]

Signed-off-by: Alessandro Comodi acomodi@antmicro.com

This is an attempt at solving the issue described in this comment: chipsalliance/python-fpga-interchange#76 (comment).

Basically, a site-thru PIP should not be allowed in the cases where the a BEL is not in the same site of the site-thru PIP. The change in this PR is rather strict, as if a net has multiple sinks in different sites, the PIP is automatically considered illegal, even though the actual sink BEL of that connection resides in the same site.

cc @gatecat

[gatecat]

Two problems I see here:

• if the problem pip is being inserted by the general router, which I think is probable, then this part of the code won't be being reached at all due to disallow_site_routing:

  nextpnr/fpga_interchange/arch.cc

  Lines 1787 to 1790 in dbcddac

  	if (disallow_site_routing && !valid_pip) {
  	// For now, if driver is not part of this site, and
  	// disallow_site_routing is set, disallow the edge.
  	return false;

• that aside, this seems like it will reject some possible valid cases where site routing will have to be used

An alternative suggested solution would be to backtrace through net->wires* and see if the current routed path has left the site yet, rejecting the path if a wire in the backtrace is outside of any sites.

• in some cases this function will be called without a complete path here, these cases should not error out but instead assume the path is valid; it will be checked again with a full path later on and that's when it can be rejected.

[acomodi]

if the problem pip is being inserted by the general router, which I think is probable, then this part of the code won't be being reached at all due to disallow_site_routing

I believe that the general router should not perform such a check, mainly because the entire site is routed through with a tile PIP, and the sink will most probably reside outside the site of the tile PIP.

that aside, this seems like it will reject some possible valid cases where site routing will have to be used

I agree, this will definetely make some valid PIP invalid, and yeah I guess backtracking could be a viable solution here. I wonder if, using backtracking, might be expensive (from a run-time point of view).

Unclear though at what stage this check should be performed.

[gatecat]

I agree, this will definetely make some valid PIP invalid, and yeah I guess backtracking could be a viable solution here. I wonder if, using backtracking, might be expensive (from a run-time point of view).

It probably wouldn't be the biggest runtime issue ATM, runtime is something that currently needs a lot more thought (it's about 50x slower than nextpnr-xilinx overall in its present state...)

If this patch does fix the problem, then I don't have a problem with it provided a comment is left detailing why it isn't quite optimal.

[acomodi]

@gatecat I have force pushed another solution that adds an additional check during site routing. This should prevent the site router to expand nodes that are in illegal paths, where an illegal path is present when a site is first entered and than left, meaning that a tile PIP should have been used instead.

[gatecat]

nitpick - "not a valid PIP"

[acomodi]

Right, fixed, also for the other non-valid PIP logs

[gatecat]

Thanks!

[acomodi]

So, I have run some more tests with this patch and unfortunately, this is not enough, as I have still hit some edge cases. There must be some other place where the illegal routes are still allowed, and I believe the possible cause might be this:

nextpnr/fpga_interchange/arch.cc

Lines 1809 to 1813 in 466de95

	if (dst_wire_data.site == bel_data.site && src_wire_data.site == bel_data.site) {
	// This is site pip for the same site as the driver, allow
	// this site pip.
	valid_pip = true;
	}

The general router is disallowed to route through a site, but this might not be true for edge cases in which the intra-site route happens to be in the same site as the driver site-wire. I am not sure why the above code is necessary though.