-
Notifications
You must be signed in to change notification settings - Fork 111
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
[ModuleInst] Fix placement behavior when requesting no overlaps #1108
Conversation
Signed-off-by: Chris Lavin <[email protected]>
} | ||
|
||
if (existingSiteInst != null && !allowOverlap) { | ||
unplace(); | ||
return false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
if (existingSiteInst != null && !allowOverlap) { | |
unplace(); | |
return false; | |
} | |
if (existingSiteInst != null) { | |
unplace(); | |
return false; | |
} | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Chris Lavin <[email protected]>
Signed-off-by: eddieh-xlnx <[email protected]>
ModuleInst.place()
has a flag to request that no overlapping modules should be allowed. There was a bug where this was not behaving as expected. This attempts to align the behavior with the intent of the method.