-
Notifications
You must be signed in to change notification settings - Fork 899
Missing para dbmodnet #9745
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
Missing para dbmodnet #9745
Changes from all commits
39ac5ac
35f274e
09730f1
0489b20
46be33b
b19079f
6832007
73558ab
b614eb5
51a1bb5
62d242f
ea8cf58
8603606
1623227
1ca316c
757e8a0
d71d23e
923a5ee
32458ec
bc799c6
bc23866
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2018,9 +2018,18 @@ void dbNetwork::visitConnectedPins(const Net* net, | |
| } | ||
| } | ||
|
|
||
| // Caution: | ||
| //- Network::highestConnectedNet(Net *net) retrieves the highest hierarchical | ||
| // net connected to the given net. | ||
| // - But `dbNetwork::highestConnectedNet(Net* net)` retrieves the corresponding | ||
| // flat net for the given net. | ||
| // - It behaves differently to cope with the issue 9724. | ||
| // - This redefinition may cause another issue later when | ||
| // `Network::highestConnectedNet(Net *net)` is used elsewhere. | ||
| const Net* dbNetwork::highestConnectedNet(Net* net) const | ||
| { | ||
| return net; | ||
| const Net* flat = findFlatNet(net); | ||
| return flat ? flat : net; | ||
| } | ||
|
|
||
| //////////////////////////////////////////////////////////////// | ||
|
|
@@ -5218,11 +5227,15 @@ Net* dbNetwork::highestNetAbove(Net* net) const | |
| } | ||
|
|
||
| if (modnet) { | ||
| // Return the flat net associated with this mod net. | ||
| // Parasitic externality checks in | ||
| // ConcreteParasiticNetwork::ensureParasiticNode compare against net_ which | ||
| // is always a flat net (set via makeParasiticNetwork). Returning the | ||
| // highest mod net causes all pin nodes on hierarchically-connected nets to | ||
| // compare unequal to net_ and be incorrectly marked as external, making | ||
| // node_count_ = 0 and crashing PRIMA in measureThresholds. | ||
| if (dbNet* related_dbnet = modnet->findRelatedNet()) { | ||
| if (odb::dbModNet* highest_modnet | ||
| = related_dbnet->findModNetInHighestHier()) { | ||
| return dbToSta(highest_modnet); // Found the highest modnet | ||
| } | ||
| return dbToSta(related_dbnet); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Net* dbNetwork::highestNetAbove that I modified is not called by ReportPath::descriptionNet(const Pin *pin). Rather it calls Network::highestNetAbove(Net *net) function. This highestNetAbove is defined virtual in the Network class
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay I see what you mean. In OpenROAD binary, ReportPath::descriptionNet() dispatches to dbNetwork::highestNetAbove(). So the opposite is guaranteed: OpenROAD will always call dbNetwork::highestNetAbove(), not Network::highestNetAbove(). Hmm.. I don't know the answer yet- I don't see any regression failures, but I will get CC to check for this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, dbNetwork::highestNetAbove() changes report_checks output (and any command using ReportPath::descriptionNet), but only for hierarchical designs with modNets — and it changes the net name displayed, not the timing values. top --> u_core (instance of "core") --> u_alu (instance of "alu") --> adder gate drives wire "sum_out"
If the same physical signal has a different modNet name at each hierarchy level as shown above, after flattening, one flat dbNet is created. Its name depends on where/how flattening named it — suppose it's u_core/alu_result (a common convention: the net retains the name from an intermediate hierarchy level).
The old behavior always showed the topmost hierarchical name — the name at the highest module boundary the signal reached. The new behavior shows the flat dbNet name, which is whatever name the net was assigned during flattening/synthesis. What is NOT affected
|
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| [INFO ODB-0227] LEF file: Nangate45/Nangate45.lef, created 22 layers, 27 vias, 135 library cells | ||
| [WARNING ORD-0011] Hierarchical flow (-hier) is currently in development and may cause multiple issues. Do not use in production environments. | ||
| Found 3 unannotated drivers. | ||
| ff_out/QN | ||
| ff_top/QN | ||
| sub_inst/ff_sub/QN | ||
| Found 0 partially unannotated drivers. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| *SPEF "IEEE 1481-1998" | ||
| *DESIGN "top" | ||
| *DATE "now" | ||
| *VENDOR "n/a" | ||
| *PROGRAM "n/a" | ||
| *VERSION "1.0" | ||
| *DESIGN_FLOW "" | ||
| *DIVIDER / | ||
| *DELIMITER : | ||
| *BUS_DELIMITER [ ] | ||
| *T_UNIT 1 NS | ||
| *C_UNIT 1 PF | ||
| *R_UNIT 1 OHM | ||
| *L_UNIT 1 HENRY | ||
|
|
||
| *D_NET clk 0.001 | ||
| *CONN | ||
| *P clk I | ||
| *I ck_buf:A I | ||
| *CAP | ||
| 1 clk 0.0005 | ||
| 2 ck_buf:A 0.0005 | ||
| *RES | ||
| 1 clk ck_buf:A 0.1 | ||
| *END | ||
|
|
||
| *D_NET clk_int 0.002 | ||
| *CONN | ||
| *I ck_buf:Z O | ||
| *I ff_top:CK I | ||
| *I ff_out:CK I | ||
| *I sub_inst/ff_sub:CK I | ||
| *CAP | ||
| 1 ck_buf:Z 0.0005 | ||
| 2 ff_top:CK 0.0005 | ||
| 3 ff_out:CK 0.0005 | ||
| 4 sub_inst/ff_sub:CK 0.0005 | ||
| *RES | ||
| 1 ck_buf:Z ff_top:CK 0.1 | ||
| 2 ck_buf:Z ff_out:CK 0.1 | ||
| 3 ck_buf:Z sub_inst/ff_sub:CK 0.1 | ||
| *END | ||
|
|
||
| *D_NET in 0.001 | ||
| *CONN | ||
| *P in I | ||
| *I ff_top:D I | ||
| *CAP | ||
| 1 in 0.0005 | ||
| 2 ff_top:D 0.0005 | ||
| *RES | ||
| 1 in ff_top:D 0.1 | ||
| *END | ||
|
|
||
| *D_NET out 0.001 | ||
| *CONN | ||
| *I ff_out:Q O | ||
| *P out O | ||
| *CAP | ||
| 1 ff_out:Q 0.0005 | ||
| 2 out 0.0005 | ||
| *RES | ||
| 1 ff_out:Q out 0.1 | ||
| *END | ||
|
|
||
| *D_NET midnet 0.001 | ||
| *CONN | ||
| *I ff_top:Q O | ||
| *I sub_inst/ff_sub:D I | ||
| *CAP | ||
| 1 ff_top:Q 0.0005 | ||
| 2 sub_inst/ff_sub:D 0.0005 | ||
| *RES | ||
| 1 ff_top:Q sub_inst/ff_sub:D 0.1 | ||
| *END | ||
|
|
||
| *D_NET intermediate 0.001 | ||
| *CONN | ||
| *I sub_inst/buf_sub:Z O | ||
| *I ff_out:D I | ||
| *CAP | ||
| 1 sub_inst/buf_sub:Z 0.0005 | ||
| 2 ff_out:D 0.0005 | ||
| *RES | ||
| 1 sub_inst/buf_sub:Z ff_out:D 0.1 | ||
| *END | ||
|
|
||
| *D_NET sub_inst/q_int 0.001 | ||
| *CONN | ||
| *I sub_inst/ff_sub:Q O | ||
| *I sub_inst/buf_sub:A I | ||
| *CAP | ||
| 1 sub_inst/ff_sub:Q 0.0005 | ||
| 2 sub_inst/buf_sub:A 0.0005 | ||
| *RES | ||
| 1 sub_inst/ff_sub:Q sub_inst/buf_sub:A 0.1 | ||
| *END |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| # Regression for PR #3194: dbNetwork::highestConnectedNet on a hier | ||
| # dbModNet must resolve to the associated flat dbNet so that | ||
| # Parasitics::findParasiticNet hits the parasitic_network_map_ entry | ||
| # inserted under the flat dbNet key. | ||
| # | ||
| # Why this test uses read_spef rather than estimate_parasitics: | ||
| # estimate_parasitics keys parasitic_network_map_ via findParasiticNet | ||
| # for BOTH insert and lookup, so the bug is self-consistent and | ||
| # invisible (insert key == lookup key, both buggy or both fixed). | ||
| # SPEF read keys insertion by flat dbNet (parser -> findNet -> flat | ||
| # at top scope) while lookup still uses findParasiticNet -> | ||
| # dbNetwork::highestConnectedNet. Pre-fix override echoed the modnet | ||
| # wrapper -> modnet key != flat key -> cache miss -> driver | ||
| # classified unannotated. Fix returns the related flat dbNet -> hit. | ||
| # | ||
| # Design (hier_highest_connected_net.v): 2-level hier with reg-to-reg | ||
| # data path crossing sub_inst. Top clk is buffered by ck_buf so the | ||
| # clock-tree driver iterated is a leaf iterm, not the top BTerm; this | ||
| # avoids the top-port bypass branch in Parasitics::findParasiticNet | ||
| # (which returns network_->net(term) directly with no | ||
| # highestConnectedNet call and is independent of PR #3194) that would | ||
| # otherwise leave clk unannotated on the fixed binary too. | ||
| # | ||
| # Hier-crossing nets (carry both flat dbNet + dbModNet on their leaf | ||
| # iterms after link_design -hier): | ||
| # clk_int : ck_buf/Z -> ff_top/CK, ff_out/CK, sub_inst/ff_sub/CK | ||
| # midnet : ff_top/Q -> sub_inst/ff_sub/D | ||
| # intermediate : sub_inst/buf_sub/Z -> ff_out/D | ||
| # | ||
| # Expected outputs: | ||
| # | ||
| # Fixed binary: | ||
| # Found 3 unannotated drivers. | ||
| # ff_out/QN | ||
| # ff_top/QN | ||
| # sub_inst/ff_sub/QN | ||
| # Found 0 partially unannotated drivers. | ||
| # | ||
| # Bug binary: | ||
| # Found 6 unannotated drivers. | ||
| # ck_buf/Z | ||
| # ff_out/QN | ||
| # ff_top/Q | ||
| # ff_top/QN | ||
| # sub_inst/buf_sub/Z | ||
| # sub_inst/ff_sub/QN | ||
| # Found 0 partially unannotated drivers. | ||
| # | ||
| # The 3 baseline entries (ff_top/QN, ff_out/QN, sub_inst/ff_sub/QN) | ||
| # are dangling DFF QN outputs -- no loads, no wire, no parasitic | ||
| # possible. The 3 bug-extras (ck_buf/Z, ff_top/Q, sub_inst/buf_sub/Z) | ||
| # are the modnet-wrapped drivers on the hier-crossing nets above: | ||
| # findParasiticNet returns their dbModNet wrapper, SPEF-inserted | ||
| # entries are keyed by flat dbNet -> map miss on bug binary, hit on | ||
| # fixed binary. | ||
| source "helpers.tcl" | ||
| read_liberty Nangate45/Nangate45_typ.lib | ||
| read_lef Nangate45/Nangate45.lef | ||
| read_verilog hier_highest_connected_net.v | ||
| link_design top -hier | ||
|
|
||
| create_clock -name clk -period 5 [get_ports clk] | ||
| set_input_delay -clock clk 0 [get_ports in] | ||
| set_output_delay -clock clk 0 [get_ports out] | ||
|
|
||
| read_spef hier_highest_connected_net.spef | ||
|
|
||
| report_parasitic_annotation -report_unannotated |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // 2-level hier reg-to-reg design. midnet/intermediate span sub_inst | ||
| // boundary so leaf iterms on those nets get both flat dbNet and | ||
| // dbModNet wrappers after link_design -hier. Clock buffer ck_buf | ||
| // isolates the top clk BTerm so driver iteration doesn't hit the | ||
| // top-BTerm bypass branch in Parasitics::findParasiticNet (which is | ||
| // independent of PR #3194 and would otherwise leave clk unannotated). | ||
|
|
||
| module sub (din, sub_clk, dout); | ||
| input din; | ||
| input sub_clk; | ||
| output dout; | ||
|
|
||
| wire q_int; | ||
|
|
||
| DFF_X1 ff_sub (.D(din), .CK(sub_clk), .Q(q_int)); | ||
| BUF_X1 buf_sub (.A(q_int), .Z(dout)); | ||
| endmodule | ||
|
|
||
| module top (in, clk, out); | ||
| input in; | ||
| input clk; | ||
| output out; | ||
|
|
||
| wire clk_int; | ||
| wire midnet; | ||
| wire intermediate; | ||
|
|
||
| BUF_X1 ck_buf (.A(clk), .Z(clk_int)); | ||
| DFF_X1 ff_top (.D(in), .CK(clk_int), .Q(midnet)); | ||
| sub sub_inst (.din(midnet), .sub_clk(clk_int), .dout(intermediate)); | ||
| DFF_X1 ff_out (.D(intermediate), .CK(clk_int), .Q(out)); | ||
| endmodule |



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.
The objective of
Network::highestConnectedNet(Net* net)is to retrieve the hierarchical net in the highest module.But the this
dbNetwork::highestConnectedNet(Net* net)finds the corresponding flat net for the given hier net, which is a different behavior.Fortunately, there is only one caller
Parasitics::findParasiticNet(const Pin *pin). So the current fix works.But if
Network::highestConnectedNet(Net* net)is used by another caller for a different purpose later, this fix can make a trouble.I don't have a better idea about how to solve this issue.
But let's leave a comment to describe this issue.
e.g.,
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.
To me the clean solution is to only ever expose hierarchical nets to STA since that seems to be what it's expecting and that's how
ConcreteNetworkimplementsNetworkThere 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.
Unfortunately that isn't possible with the way hierarchy was implemented in odb. For a net that is local to a single module we only create the flat net and skip the hierarchical one (since it would be equivalent as the net doesn't span a boundary). That complicates the API but does save memory on a flat design. The tradeoff could be revisited.
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.
@maliberty in that case shouldn't we expose dbNet to STA only for those nets local to a module, and use dbModNet otherwise? That would match ConcreteNetwork as far as STA could tell
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.
That is what we do.
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 what my
comment was referring to. We don't consistently use dbModNet for nets which have them, which is a source of perpetual trouble.
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 consider that a bug but the model remains as I stated it. I agree it would be simpler if we always allocated the modnet even if it was a duplicate of the dbNet. The downside is for a flat design it would double the net memory usage.
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'm not advocating for allocating dbmodnet in the trivial case but pointing out the bug. I understand the model.
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.
Maybe I didn't understand "in that case shouldn't we expose dbNet to STA only for those nets local to a module, and use dbModNet otherwise?" - were you asking about the model or pointing out a bug?
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 because of the context of the thread (what should highestConnectedNet do) that I thought the bug is implied