Skip to content

Conversation

@kbieganski
Copy link
Contributor

New resynthesis strategy for rmp.
It searches for an ABC script that improves timing via simulated annealing.
The script is a series of operations on ABC's internal AIG data structure.
A neighboring solution is a script with one operation added, removed, or two operations swapped.
The optimization function is defined as the worst slack.

@kbieganski kbieganski force-pushed the rmp-simulated-annealing branch from d320176 to 7024860 Compare September 25, 2025 16:53
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@maliberty maliberty requested a review from povik September 25, 2025 22:02
@maliberty
Copy link
Member

Please share results showing the effectiveness of this change.

@povik povik self-assigned this Sep 26, 2025
@kbieganski kbieganski force-pushed the rmp-simulated-annealing branch from 7024860 to 56cddd0 Compare September 29, 2025 10:28
@kbieganski
Copy link
Contributor Author

kbieganski commented Sep 29, 2025

Some results are in the tests: here, here, and here.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@povik povik left a comment

Choose a reason for hiding this comment

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

One request for rewording the documentation. The rest are nits or have to do with pre-existing code

| `-corner` | Process corner to use. |
| `-slack_threshold` | Specifies a (setup) timing slack value below which timing paths need to be analyzed for restructuring. The default value is `0`, and the allowed values are floats `[0, MAX_FLOAT]`. |
| `-seed` | Seed to use for randomness in simulated annealing. |
| `-temp` | Initial temperature for simulated annealing. The default is the required of the worst slack path. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the required arrival time on the endpoint for the worst slack path?

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the code it is. I think it would be good to say it's the required time on the endpoint in the description.

Copy link
Member

Choose a reason for hiding this comment

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

It think users will find required time to be a strange way to express their intent.

Copy link
Contributor

Choose a reason for hiding this comment

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

@maliberty They're specifying temperature but the default value for it is taken from the required time at the end of the worst path.

for (sta::Vertex* vertex : *sta->endpoints()) {
sta::Pin* pin = vertex->pin();
sta::PortDirection* direction = network->direction(vertex->pin());
if (!direction->isInput()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this check? I think we disqualify top module outputs here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is code moved from elsewhere but still I wonder about the purpose of the check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QuantamHD Do you know?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this was to defend against endpoints that are unconstrained. It's probably overly simplistic.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not the right check

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@kbieganski kbieganski force-pushed the rmp-simulated-annealing branch from 88ddf1e to 5f4ef6b Compare September 30, 2025 21:20
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@povik povik left a comment

Choose a reason for hiding this comment

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

Codewise this looks fine

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025

clang-tidy review says "All clean, LGTM! 👍"

@kbieganski kbieganski force-pushed the rmp-simulated-annealing branch 2 times, most recently from 725c336 to 41cbfd0 Compare October 1, 2025 16:51
@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025

clang-tidy review says "All clean, LGTM! 👍"

@kbieganski kbieganski force-pushed the rmp-simulated-annealing branch from 41cbfd0 to 3383d74 Compare October 1, 2025 19:45
@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025

clang-tidy review says "All clean, LGTM! 👍"

@maliberty
Copy link
Member

Please merge master which should fix the test failure


// GIA ops as lambdas
// All the magic numbers are defaults from abc/src/base/abci/abc.c
// Or from the ORFS qbc_speed script
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Or from the ORFS qbc_speed script
// Or from the ORFS abc_speed script

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

worse_iters = 0;
}

if (i > 0 && (i + 1) % 10 == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Redundant - if i == 0 then (i + 1) % 10 != 0 so no need to check i > 0

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

Signed-off-by: Krzysztof Bieganski <[email protected]>
Signed-off-by: Krzysztof Bieganski <[email protected]>
Signed-off-by: Krzysztof Bieganski <[email protected]>
Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski and others added 8 commits October 3, 2025 11:47
Signed-off-by: Krzysztof Bieganski <[email protected]>
Signed-off-by: Krzysztof Bieganski <[email protected]>
Signed-off-by: Krzysztof Bieganski <[email protected]>
Signed-off-by: Krzysztof Bieganski <[email protected]>
I give up

Signed-off-by: Krzysztof Bieganski <[email protected]>
Signed-off-by: Krzysztof Bieganski <[email protected]>
Signed-off-by: Krzysztof Bieganski <[email protected]>
Signed-off-by: Ryszard Rozak <[email protected]>
@RRozak RRozak force-pushed the rmp-simulated-annealing branch from 3383d74 to 39e886e Compare October 3, 2025 11:29
Signed-off-by: Ryszard Rozak <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2025

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2025

clang-tidy review says "All clean, LGTM! 👍"

@maliberty maliberty merged commit 7b5fbf8 into The-OpenROAD-Project:master Oct 3, 2025
13 checks passed
@maliberty
Copy link
Member

I didn't want to hold this PR up further but I think it would be good to add and eqy step to prove equivalence.

@RRozak RRozak deleted the rmp-simulated-annealing branch October 6, 2025 06:52
@RRozak
Copy link
Contributor

RRozak commented Oct 8, 2025

Could you tell how it should be added? I wanted to add similar commands to tests we added in this PR and put the annealing between them:

write_verilog_for_eqy eqy_repair_setup2 before "None"
run_equivalence_test eqy_repair_setup2 ./Nangate45/work_around_yosys/ "None"

Unfortunately it hangs. I also tried to run eqy_repair_setup2.tcl with EQUIVALENCE_CHECK env variable set to 1 and it didn't complete in 17h.

@maliberty
Copy link
Member

I have no expertise with those tools and find them obscure. I suggest asking on their GH for help.

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