-
Notifications
You must be signed in to change notification settings - Fork 602
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
rsz: makeRepeater check IO dont touch #6708
Conversation
Signed-off-by: Peter Gadfort <[email protected]>
Signed-off-by: Peter Gadfort <[email protected]>
Signed-off-by: Peter Gadfort <[email protected]>
clang-tidy review says "All clean, LGTM! 👍" |
@andyfox-rushc FYI |
I respectfully suggest checking the load_net driver pin(s) and load pins for sanity and rejecting the makeRepeater move on entry to make Repeater if it is something we cannot handle. The tricky part is defining the "axiom" for what makes a net we cannot rebuffer. The pr author has identified that the driving pin being connected to a dont touch primary input as being one such case. There are likely others. Therefore having a "legallityCheck" on the load_net and the load_pins input to the function seems to me to be a good idea so that part of the code is nicely isolated:
To help:
To see how to get the load_net driver starting from the load_net use: db_network -> getNetDriverParentModule. (as a side effect this returns the driver pin through its second argument)
Iterate through the load_pins and get the load instances.
Then do some case analysis to see if makeRepeater makes sense eg the following cases might not. Perhaps the following might apply:
1. The load_net is don't touch. (Interestingly I don't see much code that seems to check for dont touch nets)
2. The load net is hooked to a primary i/o that is don't touch.
3. All the load pins and the driver pin are don't touch.
4. There are multiple primary input drivers on the net (some kind of wired or or tri-state).
Of course 1-4 above might be junk and I am likely wrong but it seems best to me to sanity check the incoming data and not modifying the circuit rather than selectively rejecting the move after the event.
If the above strategy seems ok I am happy to provide the code to do it and then the "sanity check" routine can simply be rehacked as we find more amusing and interesting cases where makeRepeater is to be throttled.
andy
… On 02/13/2025 11:23 PM PST Matt Liberty ***@***.***> wrote:
@andyfox-rushc https://github.com/andyfox-rushc FYI
—
Reply to this email directly, view it on GitHub #6708 (comment), or unsubscribe https://github.com/notifications/unsubscribe-auth/AHRTF7UABUPEVEZ5TOERAKL2PWKX5AVCNFSM6AAAAABXCXQBGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNJYGQ3TKNZTG4.
You are receiving this because you were mentioned.Message ID: ***@***.***>
[maliberty]maliberty left a comment (The-OpenROAD-Project/OpenROAD#6708) #6708 (comment)
@andyfox-rushc https://github.com/andyfox-rushc FYI
—
Reply to this email directly, view it on GitHub #6708 (comment), or unsubscribe https://github.com/notifications/unsubscribe-auth/AHRTF7UABUPEVEZ5TOERAKL2PWKX5AVCNFSM6AAAAABXCXQBGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNJYGQ3TKNZTG4.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
src/rsz/src/RepairDesign.cc
Outdated
@@ -1850,6 +1854,30 @@ void RepairDesign::makeRepeater( | |||
} | |||
} | |||
|
|||
if (!hasInputPort(load_net) && preserve_outputs) { |
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 hasInputPort(load_net) && preserve_outputs
is true then it seems you can't insert a buffer.
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.
this is just the not(hasInputPort(load_net) || !preserve_outputs)
from below since this is checking a specific case, I'll redo that logic to make it clear
src/rsz/src/RepairDesign.cc
Outdated
@@ -2019,6 +2051,27 @@ void RepairDesign::makeRepeater( | |||
sta_->connectPin(buffer, buffer_output_port, buffer_op_net); | |||
} // case 2 | |||
|
|||
if (!connections_modified) { |
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.
When would this happen? It seems both cases modify connections.
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 the buffer is only connected to instances that are marked dont_touch then no connections would be made
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.
You can still insert a repeater in that case unless the driver is also dont-touch or a primary input
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.
you cannot, because you cannot change the net that the dont_touch buffers are attached to (thats what is failing). If you cannot break the net into two parts because all the instances on one or the other side of it are marked dont_touch then adding the buffer just results in a floating net somewhere.
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.
You just leave the loads on their current net and add a new net at the repeater input instead.
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.
In any case it is better to avoid this upstream impossible combinations
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 mode the check up to earlier, so the original code is not really modified.
Signed-off-by: Peter Gadfort <[email protected]>
Signed-off-by: Peter Gadfort <[email protected]>
86ddb1f
to
f54ffd1
Compare
Fixes: