Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent behaviour for Cell.getSitePinFromLogicalPin() #473

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Inconsistent behaviour for Cell.getSitePinFromLogicalPin() #473

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion test/src/com/xilinx/rapidwright/design/TestCell.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
* Copyright (c) 2022, Xilinx, Inc.
* Copyright (c) 2022-2023, Advanced Micro Devices, Inc.
* Copyright (c) 2022-2024, Advanced Micro Devices, Inc.
* All rights reserved.
*
* Author: Eddie Hung, Xilinx Research Labs.
Expand All @@ -24,6 +24,7 @@
package com.xilinx.rapidwright.design;


import com.xilinx.rapidwright.device.BELPin;
import com.xilinx.rapidwright.device.Device;
import com.xilinx.rapidwright.support.RapidWrightDCP;
import com.xilinx.rapidwright.util.FileTools;
Expand All @@ -32,7 +33,9 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.ValueSource;

import java.util.Arrays;
import java.util.List;

public class TestCell {
Expand Down Expand Up @@ -110,4 +113,38 @@ public void testFFRoutethruCell() {
Assertions.assertEquals(0, VivadoTools.reportRouteStatus(d).netsWithRoutingErrors);
}
}

@ParameterizedTest
@ValueSource(booleans = {true, false})
void testGetSitePinFromLogicalPin(boolean createAX) {
Design design = new Design("top", Device.PYNQ_Z1);
SiteInst si = design.createSiteInst("SLICE_X1Y0");
Net net = design.createNet("net");
SitePinInst A3 = net.createPin("A3", si);
SitePinInst AX = (createAX) ? net.createPin("AX", si) : null;

Cell ff = design.createAndPlaceCell("ff", Unisim.FDRE, "SLICE_X1Y0/AFF");

BELPin ffD = ff.getBEL().getPin("D");
for (SitePinInst spi : Arrays.asList(AX, A3)) {
if (spi == null)
continue;

Assertions.assertNull(DesignTools.getRoutedSitePin(ff, net, "D"));
if (createAX) {
// FIXME: Known broken -- see https://github.com/Xilinx/RapidWright/issues/473
Assertions.assertEquals("IN SLICE_X1Y0.AX", ff.getSitePinFromLogicalPin("D", null).toString());
Comment on lines +135 to +136
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clavin-xlnx (see PR main description first). This is where I waive the possibly-incorrect result. The question is: is it really incorrect?

Copy link
Collaborator Author

@eddieh-xlnx eddieh-xlnx Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it some more, when we have a net that reaches a site's A3 and AX input pins, and for some reason there is no intra-site routing to follow, then I think it's not unreasonable for Cell.getSitePinFromLogicalPin() to default to AX for flops.

The question in the A3-only case, should it consider a LUT routethru (where feasible) as a way of reaching that net's sink?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question in the A3-only case, it should consider a LUT routethru (where feasible) as a way of reaching that net's sink?

It seems like the behavior of Cell.getSitePinFromLogicalPin() is in question. We should probably update the Javadoc of this method once we settle on a behavior. I would say that this method should return the first routed SitePinInst if one exists. If a routed SitePinInst does not exist, it seems like it should return null.

In the case where the SitePinInst already exists, but is not routed and the SitePinInst belongs to the net connected to the logical pin on the cell, then I am ok with changing its behavior to identify what it should be. But again, I think we should also update the Javadoc of DesignTools.getRoutedSitePin() because it currently claims to return null when there is no routed pin.

I think defaulting to the *X input site pins for logical D inputs on SLICE flip flops makes sense. For unoccupied LUT inputs site pins with no other options, I am ok with those when there is no other option.

Copy link
Collaborator Author

@eddieh-xlnx eddieh-xlnx Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that this method should return the first routed SitePinInst if one exists. If a routed SitePinInst does not exist, it seems like it should return null.

My understanding is that DesignTools.getRoutedSitePin() examines intra-site routing to tell you what the current site pin is -- which the test does check returns always returns null since there is no intra-site routing. It looks like Cell.getSitePinFromLogicalPin() is for answering the question of what a future site pin could/should be -- presumably, ahead of performing any intra-site routing.

In this context, it looks like Cell.getSitePinFromLogicalPin() is working as expected, except it doesn't seem to consider LUT routethrus when looking to re-use existing site pins from the same net.

Just to be clear, calling Cell.getSitePinFromLogicalPin() on a site with no intra-site routing with the following net sinks:

  • No sinks gives null (correct)
  • {A3, AX} gives AX (correct)
  • AX gives AX (correct)
  • A3 gives null (in question, but I think I can live with this until it bites me next time).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this behavior is fine and if we want to extend the A3 case to also return A3, that is fine too. But it would appear that the Javadoc descriptions are incorrect.

} else {
Assertions.assertNull(ff.getSitePinFromLogicalPin("D", null));
}

BELPin bp = spi.getBELPin();
Assertions.assertTrue(si.routeIntraSiteNet(net, bp, ffD));

Assertions.assertEquals(bp.getName(), DesignTools.getRoutedSitePin(ff, net, "D"));
Assertions.assertEquals(spi, ff.getSitePinFromLogicalPin("D", null));

Assertions.assertTrue(si.unrouteIntraSiteNet(bp, ffD));
}
}
}
Loading