Opt in first 8 designs to use 'syn' #4270
Conversation
![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 introduces a new synthesis flow option (SYNTH_USE_SYN) that utilizes the built-in OpenROAD 'syn' tool via a new synth_syn.tcl script, updating the Makefile, metrics extraction, and various design configurations to support it. The code review feedback highlights several critical improvements for robustness: adding missing Verilog and SDC dependencies to the Makefile target to prevent breaking incremental builds, guarding environment variables (DONT_USE_CELLS and SYNTH_SLANG_ARGS) against Tcl runtime errors when undefined, and keeping vIdirsArgs as a proper Tcl list to safely handle paths with special characters.
| ifeq ($(SYNTH_USE_SYN),1) | ||
| $(eval $(call do-step,1_synth,$(TECH_LEF) $(SC_LEF) $(ADDITIONAL_LEFS) $(LIB_FILES),synth_syn)) | ||
| else | ||
| $(eval $(call do-step,1_synth,$(RESULTS_DIR)/1_2_yosys.v $(RESULTS_DIR)/1_2_yosys.sdc $(TECH_LEF) $(SC_LEF) $(ADDITIONAL_LEFS) $(LIB_FILES),synth_odb)) | ||
| endif |
There was a problem hiding this comment.
In the new syn synthesis flow (SYNTH_USE_SYN=1), the 1_synth target does not list $(VERILOG_FILES) or $(SDC_FILE) as dependencies. This breaks incremental builds, as modifying any Verilog source files or timing constraints will not trigger a re-synthesis of the design. Please add them to the dependency list.
ifeq ($(SYNTH_USE_SYN),1)
$(eval $(call do-step,1_synth,$(VERILOG_FILES) $(SDC_FILE) $(TECH_LEF) $(SC_LEF) $(ADDITIONAL_LEFS) $(LIB_FILES),synth_syn))
else
$(eval $(call do-step,1_synth,$(RESULTS_DIR)/1_2_yosys.v $(RESULTS_DIR)/1_2_yosys.sdc $(TECH_LEF) $(SC_LEF) $(ADDITIONAL_LEFS) $(LIB_FILES),synth_odb))
endif
| read_lef $lef | ||
| } | ||
| } | ||
| set_dont_use $::env(DONT_USE_CELLS) |
There was a problem hiding this comment.
The environment variable DONT_USE_CELLS is not marked as required and does not have a default value in variables.json. If it is not defined in the environment, accessing $::env(DONT_USE_CELLS) directly will throw a Tcl runtime error and crash the synthesis flow. Please guard this call using env_var_exists_and_non_empty.
if { [env_var_exists_and_non_empty DONT_USE_CELLS] } {
set_dont_use $::env(DONT_USE_CELLS)
}
| set vIdirsArgs "" | ||
| if { [env_var_exists_and_non_empty VERILOG_INCLUDE_DIRS] } { | ||
| foreach dir $::env(VERILOG_INCLUDE_DIRS) { | ||
| lappend vIdirsArgs "-I$dir" | ||
| } | ||
| set vIdirsArgs [join $vIdirsArgs] | ||
| } |
There was a problem hiding this comment.
Instead of joining the vIdirsArgs list into a space-separated string and then using expansion ({*}) to split it back, it is cleaner and safer to keep vIdirsArgs as a proper Tcl list. This avoids potential issues with paths containing special characters or spaces.
set vIdirsArgs [list]
if { [env_var_exists_and_non_empty VERILOG_INCLUDE_DIRS] } {
foreach dir $::env(VERILOG_INCLUDE_DIRS) {
lappend vIdirsArgs "-I$dir"
}
}
| } | ||
| } | ||
|
|
||
| lappend elaborate_args {*}$::env(SYNTH_SLANG_ARGS) |
There was a problem hiding this comment.
Directly accessing $::env(SYNTH_SLANG_ARGS) can cause a runtime error if the variable is not exported to the environment. It is safer and more consistent with other environment variables to use the env_var_or_empty helper.
lappend elaborate_args {*}[env_var_or_empty SYNTH_SLANG_ARGS]
openroad-ci
force-pushed
the
secure-syn-bringup
branch
2 times, most recently
from
675bd40 to
af686ba
Compare
Signed-off-by: Martin Povišer <povik@cutebit.org>
Signed-off-by: Martin Povišer <povik@cutebit.org>
Signed-off-by: Martin Povišer <povik@cutebit.org>
openroad-ci
force-pushed
the
secure-syn-bringup
branch
from
af686ba to
c298a08
Compare
Signed-off-by: Martin Povišer <povik@cutebit.org>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
2 participants
Updated Rules
designs/asap7/aes/rules-base.json updates:
designs/asap7/aes_lvt/rules-base.json updates:
[WARNING] Multiple clocks not supported. Will use first clock: mrx_clk_pad_i: 300.0000.
[WARNING] Multiple clocks not supported. Will use first clock: mrx_clk_pad_i: 300.0000.
designs/asap7/gcd/rules-base.json updates:
designs/asap7/jpeg/rules-base.json updates:
designs/asap7/jpeg_lvt/rules-base.json updates:
[WARNING] Multiple clocks not supported. Will use first clock: clk: 333.0000.
designs/asap7/riscv32i-mock-sram/rules-base.json updates:
designs/gf180/uart-blocks/rules-base.json updates:
designs/ihp-sg13g2/aes/rules-base.json updates:
designs/ihp-sg13g2/gcd/rules-base.json updates:
designs/ihp-sg13g2/i2c-gpio-expander/rules-base.json updates:
designs/ihp-sg13g2/jpeg/rules-base.json updates:
designs/nangate45/aes/rules-base.json updates:
designs/nangate45/ariane133/rules-base.json updates:
designs/nangate45/bp_be_top/rules-base.json updates:
designs/nangate45/bp_fe_top/rules-base.json updates:
designs/nangate45/dynamic_node/rules-base.json updates:
[WARNING] Multiple clocks not supported. Will use first clock: clk_i: 3.0000.
designs/nangate45/swerv/rules-base.json updates:
designs/nangate45/swerv_wrapper/rules-base.json updates:
designs/sky130hd/chameleon/rules-base.json updates:
designs/sky130hd/gcd/rules-base.json updates:
designs/sky130hd/jpeg/rules-base.json updates:
[WARNING] Multiple clocks not supported. Will use first clock: ext_clk: 15.0000.
designs/sky130hd/microwatt/rules-base.json updates:
designs/sky130hd/riscv32i/rules-base.json updates:
designs/sky130hs/ibex/rules-base.json updates:
designs/sky130hs/riscv32i/rules-base.json updates:
Messages from CI
[INFO] asap7/minimal not included in CI.
[INFO] gf12 not included in the update.
[INFO] gf55 not included in the update.
[INFO] nangate45/bp_quad not included in CI.