Skip to content

Comments

synth: Rm .lib preprocess after tools improved#3419

Merged
maliberty merged 2 commits intoThe-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:secure-retire-mergelib
Sep 18, 2025
Merged

synth: Rm .lib preprocess after tools improved#3419
maliberty merged 2 commits intoThe-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:secure-retire-mergelib

Conversation

@openroad-ci
Copy link
Collaborator

No description provided.

@povik
Copy link
Contributor

povik commented Aug 21, 2025

Supersedes #1807, #2619

Closes #3241

Comment on lines -284 to +262
do-yosys: $(DONT_USE_SC_LIB)
do-yosys: yosys-dependencies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding a dependency here? @oharboe ok?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long story, but I dont want these dependencies to be bazel artifacts.

This step massages exisiting artifacts and produces e.g. merged.lib, which is quite large.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because most of the dependencies grouped as yosys-dependencies are in fact required by do-yosys

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I dont want these dependencies to be bazel artifacts.

@oharboe I'm not sure what that implies, can we add the make dependency?

@maliberty
Copy link
Member

I'm glad to be rid of this cruft.

Copy link
Collaborator

@oharboe oharboe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing on the road: this is a great simplification!

There is a chance it will break bazel-orfs, but as long as we can collaborate on this post merge, I say merge.

@povik povik requested a review from maliberty August 21, 2025 19:39
@maliberty
Copy link
Member

A few test failures to address

@povik povik mentioned this pull request Aug 25, 2025
@povik
Copy link
Contributor

povik commented Aug 26, 2025

The failures are due to abc failing to read libraries produced with sta's write_timing_model. I've posted a fix upstream berkeley-abc/abc#437

@oharboe
Copy link
Collaborator

oharboe commented Aug 30, 2025

The failures are due to abc failing to read libraries produced with sta's write_timing_model. I've posted a fix upstream berkeley-abc/abc#437

Merged.

Purely for my planning purposes, I have to delete some cruft in bazel-orfs when this is merged, what are the next steps and when will this merge? After next yosys update?

@povik
Copy link
Contributor

povik commented Aug 30, 2025

Yosys needs to update their abc, and we need to update our yosys. I’m guessing it will take 2 or 6 weeks.

Signed-off-by: Martin Povišer <povik@cutebit.org>
@openroad-ci openroad-ci force-pushed the secure-retire-mergelib branch from f4a2f41 to 644ff70 Compare September 18, 2025 09:27
designs/gf180/aes-hybrid/rules-base.json updates:
| Metric                                        | Old      | New      | Type     |
| ------                                        | ---      | ---      | ----     |
| synth__design__instance__area__stdcell        | 624425.81 | 493531.52 | Tighten  |
| placeopt__design__instance__area              |   798562 |   657615 | Tighten  |
| placeopt__design__instance__count__stdcell    |    22568 |    22088 | Tighten  |
| globalroute__antenna_diodes_count             |        3 |        2 | Tighten  |
| detailedroute__route__wirelength              |  1623163 |  1503800 | Tighten  |
| detailedroute__antenna_diodes_count           |        5 |        9 | Failing  |
| finish__timing__setup__ws                     |    -1.16 |    -1.35 | Failing  |
| finish__design__instance__area                |   803898 |   779709 | Tighten  |

Signed-off-by: Martin Povišer <povik@cutebit.org>
@povik
Copy link
Contributor

povik commented Sep 18, 2025

designs/gf180/aes-hybrid/rules-base.json updates:

Metric Old New Type
synth__design__instance__area__stdcell 624425.81 493531.52 Tighten
placeopt__design__instance__area 798562 657615 Tighten
placeopt__design__instance__count__stdcell 22568 22088 Tighten
globalroute__antenna_diodes_count 3 2 Tighten
detailedroute__route__wirelength 1623163 1503800 Tighten
detailedroute__antenna_diodes_count 5 9 Failing
finish__timing__setup__ws -1.16 -1.35 Failing
finish__design__instance__area 803898 779709 Tighten

@maliberty
Copy link
Member

why does removing the preprocessing change the results?

@povik
Copy link
Contributor

povik commented Sep 18, 2025

I'm not sure. Should I find out where the divergence starts?

@maliberty
Copy link
Member

I think so if it is unexplained.

@povik
Copy link
Contributor

povik commented Sep 18, 2025

I've found the first divergence is picking different technology flops to map to. Previously synthesis would use 9t cells and now it uses 7t. I'm not sure why it chose 9t previously since Yosys should pick those with lower area.

74401c74401
<   cell \gf180mcu_fd_sc_mcu9t5v0__dffq_2 \u0.w[3][12]$_DFF_P_
---
>   cell \gf180mcu_fd_sc_mcu7t5v0__dffq_2 \u0.w[3][12]$_DFF_P_

@povik
Copy link
Contributor

povik commented Sep 18, 2025

The 7t flop has lower area and the same number of pins.

attribute \area "68.051200"
module \gf180mcu_fd_sc_mcu7t5v0__dffq_2
  wire output 3 \Q
  wire input 2 \D
  wire input 1 \CLK
end

attribute \area "84.672000"
module \gf180mcu_fd_sc_mcu9t5v0__dffq_2
  wire output 3 \Q
  wire input 2 \D
  wire input 1 \CLK
end

@povik
Copy link
Contributor

povik commented Sep 18, 2025

I think I understand it now. Previously only the first library from LIB_FILES would get used in synthesis. asap7 worked around this by having the merge step at the platform level. There was no such merge step for gf180/aes-hybrid despite supplying both the 9t and 7t libraries.

@povik
Copy link
Contributor

povik commented Sep 18, 2025

@maliberty I think all is good to review and merge.

@maliberty maliberty enabled auto-merge September 18, 2025 16:32
@maliberty maliberty merged commit bb27eaa into The-OpenROAD-Project:master Sep 18, 2025
8 checks passed
@maliberty maliberty deleted the secure-retire-mergelib branch September 18, 2025 17:38
@maliberty
Copy link
Member

That test case is a hack as there is no public PDK with hybrid rows.

@rovinski
Copy link
Collaborator

rovinski commented Sep 18, 2025

Not sure I can convey how happy it makes me to see this merged 😅
Thank you!!!

@povik
Copy link
Contributor

povik commented Sep 19, 2025

Thanks to you and @widlarizer for prior work in this direction.

@oharboe
Copy link
Collaborator

oharboe commented Sep 19, 2025

bazel-orfs updated

😌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants