synth: single writer for 1_synth.sdc#4333
Conversation
1_synth.sdc had two writers: yosys' synth.tcl copied the user's SDC_FILE into place (a leftover from before SDC canonicalization existed, "one day a more sophisticated synthesis..."), then synth_odb.tcl overwrote it in place with the OpenSTA-canonicalized version. cp preserves the source's mode bits, so when SDC_FILE is a read-only build input (bazel RBE), the copy is read-only and the canonicalization write fails. Locally sources are writable, so this only accidentally worked. Drop the copies from synth.tcl and synth_preamble.tcl: synth_odb.tcl/synth_syn.tcl are the sole writers of 1_synth.sdc. 1_2_yosys.sdc (copied by the do-yosys make recipe) remains the raw SDC the yosys stage hands to synth_odb.tcl. Also write 1_synth.odb/.sdc unconditionally in synth_odb.tcl and synth_syn.tcl instead of via the orfs_write_* wrappers: producing those files is the step's contract, and the WRITE_ODB_AND_SDC_EACH_STAGE=0 no-op silently left downstream stages consuming the stale, non-canonicalized copy. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the synthesis scripts to ensure that 1_synth.sdc is solely written and canonicalized by OpenSTA using write_sdc -no_timestamp in synth_odb.tcl and synth_syn.tcl, rather than copying the original SDC file in synth.tcl and synth_preamble.tcl. It also replaces orfs_write_db and orfs_write_sdc with direct log_cmd write_db and log_cmd write_sdc calls. There are no review comments, so I have no feedback to provide.
|
@maliberty TL;DR orfs_write_db is not needed/useful for synthesis, because synthesis is the first stage. |
|
@codex review |
|
I don't follow your comment about WRITE_ODB_AND_SDC_EACH_STAGE. |
We can now create a single .tcl for all openroad stages reusing the scripts, WRITE_ODB_AND_SDC_EACH_STAGE is part of that mechanics. It has been a while, I forget the details. The code isn't complicated, just read it a few lines + you have a floorplan+place joined script that you refer to occasionally, so I'm pretty sure you're aware of the feature. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
I am aware of the feature and it seems like this PR breaks it. I don't follow your justification for doing so. |
So this feature has no automated tests in CI? |
|
No as with a great many things in ORFS |
Well... if we care about a feature, we must have tests and tests are so much cheaper than they used to be. I'll see about wiring up a bazel driven test in ORFS. I know, I know... CI for |
2 participants
1_synth.sdchad two writers: yosys'synth.tclcopied the user'sSDC_FILEinto place (a leftover from before SDC canonicalization existed, "one day a more sophisticated synthesis..."), thensynth_odb.tcloverwrote it in place with the OpenSTA-canonicalized version.cppreserves the source's mode bits, so whenSDC_FILEis a read-only build input (bazel RBE), the copy is read-only and the canonicalization write fails — see OpenROAD #10818, which papered over this withchmod +w. On writable local checkouts it only accidentally worked.synth.tclandsynth_preamble.tcl:synth_odb.tcl/synth_syn.tclare the sole writers of1_synth.sdc.1_2_yosys.sdc(copied by thedo-yosysmake recipe) remains the raw SDC the yosys stage hands tosynth_odb.tcl.1_synth.odb/.sdcunconditionally insynth_odb.tclandsynth_syn.tclinstead of via theorfs_write_*wrappers: producing those files is the step's contract, and theWRITE_ODB_AND_SDC_EACH_STAGE=0no-op silently left downstream stages consuming the stale, non-canonicalized copy.bazel-orfs carries this same change as a patch (bazel-orfs#754) until this PR lands; the patch is dropped at the next ORFS bump there.
🤖 Generated with Claude Code