Skip to content

Opt in first 8 designs to use 'syn' #4270

Draft
povik wants to merge 3 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:secure-syn-bringup
Draft

Opt in first 8 designs to use 'syn' #4270
povik wants to merge 3 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:secure-syn-bringup

Conversation

@povik
Copy link
Copy Markdown
Contributor

@povik povik commented Jun 1, 2026

No description provided.

@povik povik self-assigned this Jun 1, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread flow/Makefile
Comment on lines +407 to +411
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
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

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

Comment thread flow/scripts/synth_syn.tcl Outdated
read_lef $lef
}
}
set_dont_use $::env(DONT_USE_CELLS)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

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)
}

Comment on lines +18 to +24
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]
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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]

@povik povik marked this pull request as draft June 1, 2026 13:51
@openroad-ci openroad-ci force-pushed the secure-syn-bringup branch 2 times, most recently from 675bd40 to af686ba Compare June 1, 2026 15:43
povik added 3 commits June 3, 2026 16:17
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 openroad-ci force-pushed the secure-syn-bringup branch from af686ba to c298a08 Compare June 3, 2026 14:26
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.

1 participant