-
Notifications
You must be signed in to change notification settings - Fork 108
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
[DesignTools.deletePblock()]: utility to delete a pblock #1020
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Licheng-Guo <[email protected]>
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.
Thanks for the contribution @Licheng-Guo! Can you give a motivating use-case for when this method is useful? I think I'd like to see a testcase as part of this PR so it's no longer "untested".
@clavin-xlnx Since DesignTools
is already so big, and since there could be more manipulation of the XDCs, perhaps we should create a new ConstraintTools
class?
* @param design The design to remove the pblock from. | ||
* @param pblockName The name of the pblock to remove. | ||
*/ | ||
public static void deletePblock(Design design, String pblockName) { |
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.
I would suggest calling it something more clear:
public static void deletePblock(Design design, String pblockName) { | |
public static void removeConstraintsReferencingPBlock(Design design, String pblockName) { |
boolean containsPblockCommand = pblockCommands.stream().anyMatch(line::contains); | ||
boolean containsPblockName = line.contains(pblockName); |
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.
Using contains
seems quite brittle. Given pblockName
is foo
, the following would match:
create_pblock foobar
... [get_pblocks notfoo]
add_cells_to_pblock ... [get_cells foo]
Now the robustness of this is certainly limited by RapidWright not having a XDC parser, but I would still prefer to see some stricter matching, perhaps using regex.
The current design merger does not handle XDCs, thus the final output has obsolete constraints. One simple hack for now is to delete the ones no longer needed.
I can put a test DCP into RapidWrightDCP repo and add a test here
I think this is a good idea, we might need more XDC manipulation when stitching complex designs. I can move the utility over there |
Sure, I think a |
Utility to delete a pblock.
Only tested on Vivado-generated pblocks, i.e., those existing in an out-of-box Vivado DCP