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

[ModuleInst] Fix placement behavior when requesting no overlaps #1108

Merged
merged 3 commits into from
Nov 27, 2024
Merged

[ModuleInst] Fix placement behavior when requesting no overlaps #1108

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a4103d
Fix no overlap behavior
clavin-xlnx Nov 19, 2024
7b29ab0
Simplify code
clavin-xlnx Nov 27, 2024
1d1cece
Update src/com/xilinx/rapidwright/design/ModuleInst.java
eddieh-xlnx Nov 27, 2024
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
8 changes: 5 additions & 3 deletions src/com/xilinx/rapidwright/design/ModuleInst.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,6 @@
import com.xilinx.rapidwright.device.Site;
import com.xilinx.rapidwright.device.SiteTypeEnum;
import com.xilinx.rapidwright.device.Tile;
import com.xilinx.rapidwright.edif.EDIFNet;
import com.xilinx.rapidwright.edif.EDIFPortInst;
import com.xilinx.rapidwright.edif.EDIFTools;
import com.xilinx.rapidwright.util.MessageGenerator;
import com.xilinx.rapidwright.util.Utils;

Expand Down Expand Up @@ -358,6 +355,11 @@ public boolean place(Site newAnchorSite, boolean skipIncompatible, boolean allow
}
}

if (existingSiteInst != null && !allowOverlap) {
unplace();
return false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
if (existingSiteInst != null && !allowOverlap) {
unplace();
return false;
}
if (existingSiteInst != null) {
unplace();
return false;
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear to me why this early exit is needed given the same action is performed on line 374--375 (which by observation would only trigger for existingSiteInst != null && originalSites == null && !skipIncompatible).

It no longer triggers on the latest version of the array generation code this was created for.

Copy link
Member Author

@clavin-xlnx clavin-xlnx Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If skipIncompatible==true, this needs to be checked earlier otherwise there is a failure. It looks like in the array generation code this is true, but should probably be set to false. It looks like in the latest version of the code, the placement attempt stride has changed and does not trigger the issue. I double checked and it still does for me in the initial version.

eddieh-xlnx marked this conversation as resolved.
Show resolved Hide resolved
if (newSite == null || existingSiteInst != null) {
//MessageGenerator.briefError("ERROR: No matching site found." +
// " (Template Site:" + templateSite.getName() +
Expand Down
Loading