drt: allow NDR auto-taper to be disabled per net#10805
Conversation
There was a problem hiding this comment.
Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:
- Contribution Guide: https://openroad.readthedocs.io/en/latest/contrib/contributing.html
- Build Instructions: https://openroad.readthedocs.io/en/latest/contrib/BuildWithCMake.html
Please ensure:
- CI passes
- Code is properly formatted
- Tests are included where applicable
A maintainer will review shortly!
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Signed-off-by: Simon Dorrer <simon.dorrer@jku.at>
9a79975 to
6f6803d
Compare
|
I'm not sure what a "pad trace" refers to but dbSigType can be used to detect analog nets. Does that suffice or is a new bit really needed? |
| return false; | ||
| } | ||
|
|
||
| bool UniqueInsts::isNoAutoTaperNDRInst(frInst* inst) const |
There was a problem hiding this comment.
I'm not sure about this one. Does that mean that, as soon as an instance is connected to a net with auto-taper disabled, that the entire instance has auto taper disabled?
Also, why not use isNDRInst(inst) instead of net->getNondefaultRule() as in the original code?
There was a problem hiding this comment.
isNDRInst(inst) only tells us whether the instance touches some NDR net. It reduces the per-terminal information to a single bool, so it can't tell us whether the same net is both NDR and has auto-taper suppressed.
Here I need that conjunction on one net. Take an instance with net A (NDR, taper enabled) and net B (auto-taper disabled, but no NDR): isNDRInst(inst) would be true (because of A) and "the instance has an auto-taper-disabled net" would also be true (because of B), yet no single net qualifies. So the instance should not be treated as no-auto-taper. Auto-taper only affects NDR nets, so the per-net flag is only meaningful when the same net also carries an NDR.
That's why the loop checks net->getNondefaultRule() && (!AUTO_TAPER_NDR_NETS || net->isAutoTaperDisabled()) together per terminal, rather than combining two instance-level predicates.
There was a problem hiding this comment.
However, the condition was duplicated. I've factored it into frNet::isAutoTaperSuppressed(bool auto_taper_enabled) and reused it at all three sites. Here in isNoAutoTaperNDRInst, and the two in FlexPA_acc_point. So the predicate now lives in one place. Behaviour is unchanged. See commit e638dd6
There was a problem hiding this comment.
To your other question, "Does that mean that, as soon as an instance is connected to a net with auto-taper disabled, that the entire instance has auto taper disabled?":
Yes, for pin-access purposes, if an instance touches at least one NDR net whose auto-taper is suppressed, the whole instance gets its own unique class. That's intentional and matches the pre-existing behaviour: the line this replaced was if (!AUTO_TAPER_NDR_NETS && isNDRInst(inst)), which likewise classified the entire instance. Pin-access patterns are computed per unique-instance-class, so a no-taper NDR instance needs its own class to get access points computed against the untapered (full-width NDR) geometry. The actual tapering still happens per-net/per-pin during routing. This only affects how instances are grouped for pin access.
There was a problem hiding this comment.
Alright. So as long as you can have instances with both non-NDR nets and NDR nets connected to them, it with tapering individually enabled, this should be fine.
Address review: the double negative ("disable" in the command name plus -disable/-enable flags) was confusing. Rename to `set_routing_auto_taper` and require exactly one of -enable/-disable (no default), so it reads as a direct on/off switch. Behavior and the odb flag (`disable_auto_taper`) are unchanged.
…essed` Address review from @mole99: the 'NDR net whose auto-taper is suppressed' predicate was duplicated inline in three places (FlexPA_unique once, FlexPA_acc_point twice). Extract it into `frNet::isAutoTaperSuppressed(bool auto_taper_enabled)` and reuse it. Behavior-preserving.
Good question. I asked Claude to help me with this. This is his thinking summarized. What do you say? Let me clarify "pad trace" first. In top-level chip assembly (padframe / chip integration), the wide NDR connections from I/O pads to the core (or pad-to-pad), must keep their full NDR width all the way to the pad/pin rather than taper. These are typically I looked at using dbSigType, but I don't think it covers the cases here: |
|
@mole99 @maliberty I tried to answer your review as well as possible. If you are not happy with this solution, we can further discuss it at FSiC in two days. Just let me know. However, we really would appreciate this feature. Especially for the HeiChips 2026 in the first week of August, we would need this feature. Thanks! |
Address review: the double negative ("disable" in the command name plus -disable/-enable flags) was confusing. Rename to `set_routing_auto_taper` and require exactly one of -enable/-disable (no default), so it reads as a direct on/off switch. Behavior and the odb flag (`disable_auto_taper`) are unchanged.
Signed-off-by: Simon Dorrer <simon.dorrer@jku.at>
…essed` Address review from @mole99: the 'NDR net whose auto-taper is suppressed' predicate was duplicated inline in three places (FlexPA_unique once, FlexPA_acc_point twice). Extract it into `frNet::isAutoTaperSuppressed(bool auto_taper_enabled)` and reuse it. Behavior-preserving. Signed-off-by: Simon Dorrer <simon.dorrer@jku.at>
…imi1505/OpenROAD into drt-disable-auto-taper-per-net
| } | ||
| } | ||
|
|
||
| sta::define_cmd_args "set_routing_auto_taper" \ |
There was a problem hiding this comment.
Add documentation to the odb/README
| if { !([info exists keys(-net)] ^ [info exists flags(-all_clocks)]) } { | ||
| utl::error ORD 1016 "Either -net or -all_clocks need to be defined." | ||
| } | ||
| if { [info exists flags(-enable)] == [info exists flags(-disable)] } { | ||
| utl::error ORD 1017 "Exactly one of -enable or -disable must be specified." | ||
| } |
There was a problem hiding this comment.
Why use two different idioms and different words (Either vs Exactly one) for the same one-hot style check?
| # nets without recompiling. Use -enable to restore the default behavior. | ||
| proc set_routing_auto_taper { args } { | ||
| sta::parse_key_args "set_routing_auto_taper" args \ | ||
| keys {-net} flags {-all_clocks -enable -disable} |
| bool disableAutoTaper(); | ||
|
|
||
| void setDisableAutoTaper(bool disable_auto_taper); |
There was a problem hiding this comment.
Using positive names avoids double negatives like setDisableAutoTaper(false) which just means setAutoTaper() [using bool enable=true arg]
Summary
Adds runtime, per-net control over the detailed router's auto-tapering of non-default-rule (NDR) nets. By default, DRT tapers wide NDR wires down to minimum width near pin connections. Needed for digital pins, but unwanted for wide analog/pad traces that must keep full width all the way to the pin.
Previously the only control was the compile-time
AUTO_TAPER_NDR_NETSglobal. This exposes it per net at runtime, no recompile required.Addresses #9995 and #10032.
Changes
dbNetflagdisable_auto_taper(disableAutoTaper()/setDisableAutoTaper()), with a schema-revision bump.frNet::isAutoTaperDisabled,drNet::autoTaperEnabled).set_routing_disable_auto_taper (-net <name> | -all_clocks) [-disable] [-enable].ndr_no_auto_taper.Notes
-enablerestores the default.masterwith review fixes (schema revision, split include,ord::get_db_block).Testing
Verified end-to-end via LibreLane on the IHP sg13g2 AMS chip template:
counter): routes cleanly with no regression when the option is unused (the default path is behaviour-preserving).Config comparison:
DRT_DISABLE_AUTO_TAPER: https://github.com/iic-jku/ihp-sg13g2-ams-chip-template/tree/mainDRT_DISABLE_AUTO_TAPER: https://github.com/iic-jku/ihp-sg13g2-ams-chip-template/tree/fixed-auto-taperCompanion LibreLane PR: librelane/librelane#978