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.

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

Conversation

eddieh-xlnx
Copy link
Collaborator

@eddieh-xlnx eddieh-xlnx commented Jul 14, 2022

When there is only one SitePinInst (A3), behaviour is as expected.

Cell.getSitePinFromLogicalPin() for a FF cell returns null when no intra-site routing exists, and returns A3 when routed.

When there are two SitePinInst-s -- A3 and AX -- then:

  1. Without any intra-site routing, AX is returned. Incorrect?

After disabling the assertion catching 1:

  1. When intra-site routing made for AX, AX is correctly returned.
  2. When intra-site routing made for A3, AX is returned. Incorrect. Now correct as of 425d89e.

@github-actions
Copy link

github-actions bot commented Jul 14, 2022

Unit Test Results

  33 files    33 suites   9m 59s ⏱️
533 tests 526 ✔️ 6 💤 1 ❌

For more details on these failures, see this check.

Results for commit 8db9ffb.

♻️ This comment has been updated with latest results.

Signed-off-by: Eddie Hung <eddie.hung@amd.com>
Comment on lines +135 to +136
// FIXME: Known broken -- see https://github.com/Xilinx/RapidWright/issues/473
Assertions.assertEquals("IN SLICE_X1Y0.AX", ff.getSitePinFromLogicalPin("D", null).toString());
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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants