Skip to content

Conversation

@openroad-ci
Copy link
Collaborator

No description provided.

Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Copy link
Contributor

@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 is a great step towards improving the build system by narrowing data dependencies for tests. This will improve build times and correctness. I've identified several opportunities to further refactor the new dependency lists in the BUILD files to reduce duplication and improve maintainability by using variables for common sets of dependencies. My suggestions are in src/ant/test/BUILD, src/cts/test/BUILD, src/dbSta/test/BUILD, src/dft/test/BUILD, src/dpl/test/BUILD, and src/mpl/test/BUILD.

Comment on lines +44 to +62
"check_api1": [
"merged_spacing.lef",
"sw130_random.def",
],
"check_drt1": [
"merged_spacing.lef",
"sw130_random.def",
],
"check_grt1": [
"gcd_sky130.def",
"sky130hs/sky130hs_tt.lib",
"sky130hs/sky130hs_std_cell.lef",
"sky130hs/sky130hs.tlef",
],
"no-check_grt1": [
"gcd_sky130.def",
"sky130hs/sky130hs_tt.lib",
"sky130hs/sky130hs_std_cell.lef",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve maintainability and reduce duplication, you can define variables for common dependency lists and reuse them.

For example, you can define these variables before the list comprehension at line 32:

CHECK_API_DRT_DEPS = [
    "merged_spacing.lef",
    "sw130_random.def",
]
NO_CHECK_GRT1_DEPS = [
    "gcd_sky130.def",
    "sky130hs/sky130hs_tt.lib",
    "sky130hs/sky130hs_std_cell.lef",
]

Then, you can use them in the dictionary like this:

Suggested change
"check_api1": [
"merged_spacing.lef",
"sw130_random.def",
],
"check_drt1": [
"merged_spacing.lef",
"sw130_random.def",
],
"check_grt1": [
"gcd_sky130.def",
"sky130hs/sky130hs_tt.lib",
"sky130hs/sky130hs_std_cell.lef",
"sky130hs/sky130hs.tlef",
],
"no-check_grt1": [
"gcd_sky130.def",
"sky130hs/sky130hs_tt.lib",
"sky130hs/sky130hs_std_cell.lef",
],
"check_api1": CHECK_API_DRT_DEPS,
"check_drt1": CHECK_API_DRT_DEPS,
"check_grt1": NO_CHECK_GRT1_DEPS + ["sky130hs/sky130hs.tlef"],
"no-check_grt1": NO_CHECK_GRT1_DEPS,

Comment on lines +30 to +48
"aes_nangate45": [
"Nangate45/Nangate45.lef",
"Nangate45/Nangate45_stdcell.lef",
"Nangate45/Nangate45_typ.lib",
],
"countdown_asap7": [
"asap7/asap7_tech_1x_201209.lef",
"asap7/asap7sc7p5t_28_R_1x_220121a.lef",
"asap7/asap7sc7p5t_AO_RVT_FF_nldm_211120.lib.gz",
"asap7/asap7sc7p5t_INVBUF_RVT_FF_nldm_220122.lib.gz",
"asap7/asap7sc7p5t_OA_RVT_FF_nldm_211120.lib.gz",
"asap7/asap7sc7p5t_SEQ_RVT_FF_nldm_220123.lib",
"asap7/asap7sc7p5t_SIMPLE_RVT_FF_nldm_211120.lib.gz",
],
"ibex_sky130hd": [
"sky130hd/sky130_fd_sc_hd__ss_n40C_1v40.lib",
"sky130hd/sky130hd.tlef",
"sky130hd/sky130hd_std_cell.lef",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve maintainability and reduce duplication, you could define variables for the dependency lists, especially for countdown_asap7 which has many files. This would make the BUILD file cleaner and easier to manage.

For example:

AES_NANGATE45_DEPS = [
    "Nangate45/Nangate45.lef",
    "Nangate45/Nangate45_stdcell.lef",
    "Nangate45/Nangate45_typ.lib",
]
COUNTDOWN_ASAP7_DEPS = [
    "asap7/asap7_tech_1x_201209.lef",
    "asap7/asap7sc7p5t_28_R_1x_220121a.lef",
    "asap7/asap7sc7p5t_AO_RVT_FF_nldm_211120.lib.gz",
    "asap7/asap7sc7p5t_INVBUF_RVT_FF_nldm_220122.lib.gz",
    "asap7/asap7sc7p5t_OA_RVT_FF_nldm_211120.lib.gz",
    "asap7/asap7sc7p5t_SEQ_RVT_FF_nldm_220123.lib",
    "asap7/asap7sc7p5t_SIMPLE_RVT_FF_nldm_211120.lib.gz",
]
IBEX_SKY130HD_DEPS = [
    "sky130hd/sky130_fd_sc_hd__ss_n40C_1v40.lib",
    "sky130hd/sky130hd.tlef",
    "sky130hd/sky130hd_std_cell.lef",
]

Then use these variables in the dictionary.

Comment on lines +93 to +205
"array": [
"array_tile.lef",
"array_tile.lib",
],
"array_dummy": [
"array_tile.lef",
"array_tile.lib",
],
"array_full_flow": [
"array_tile.lef",
"array_tile.lib",
],
"array_ins_delay": [
"array_tile.lef",
"array_tile_ins_delay.lib",
],
"array_no_blockages": [
"array_tile.lef",
"array_tile.lib",
],
"array_repair_clock_nets": [
"array_tile.lef",
"array_tile.lib",
],
"check_buffer_inference1": [
"ModNangate45/ModNangate45_typ.lib",
"check_buffers.def",
],
"check_buffer_inference2": [
"ModNangate45/ModNangate45.lef",
"check_buffers.def",
],
"check_buffer_inference3": [
"check_buffers.def",
],
"check_charBuf": [
"16sinks.def",
],
"check_max_fanout1": [
":ihp_resources",
],
"check_max_fanout2": [
"check_buffers.def",
],
"check_max_fanout3": [
":ihp_resources",
],
"check_wire_rc_cts": [
"check_buffers.def",
],
"dummy_load": [
"check_buffers.def",
],
"find_clock": [
"16sinks.def",
],
"find_clock_pad": [
"pad.lef",
"pad.lib",
],
"gated_clock5": [
"array_tile_ins_delay.lef",
"array_tile_ins_delay.lib",
],
"hier_insertion_delay": [
"array_tile_ins_delay.lef",
"array_tile_ins_delay.lib",
],
"insertion_delay": [
"array_tile_ins_delay.lef",
"array_tile_ins_delay.lib",
],
"inverters": [
"array_tile_ins_delay.lef",
"array_tile_ins_delay.lib",
],
"lvt_lib": [
"cts-helpers.tcl",
"Nangate45/Nangate45_lvt.lef",
"Nangate45/Nangate45_lvt.lib",
],
"max_cap": [
"//src/rsz/test:hi_fanout.tcl",
"sky130hs/sky130hs.rc",
"sky130hs/sky130hs.tlef",
"sky130hs/sky130hs_std_cell.lef",
"sky130hs/sky130hs_tt.lib",
],
"no_clocks": [
"no_clock.def",
],
"post_cts_opt": [
"cts-helpers.tcl",
],
"simple_test": [
"16sinks.def",
"simple_test_out.defok",
],
"simple_test_clustered": [
"cts-helpers.tcl",
],
"simple_test_clustered_max_cap": [
"cts-helpers.tcl",
],
"simple_test_hier": [
"simple_test_hier_out.vok",
],
"skip_nets": [
"gated_clock2.def",
],
"twice": [
"16sinks.def",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This dictionary has a lot of duplicated dependency lists. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, you can define variables for these common lists before the list comprehension at line 86.

Here are some suggestions for variables:

ARRAY_DEPS = [
    "array_tile.lef",
    "array_tile.lib",
]
ARRAY_INS_DELAY_DEPS = [
    "array_tile_ins_delay.lef",
    "array_tile_ins_delay.lib",
]
CHECK_BUFFERS_DEF_DEP = ["check_buffers.def"]
SINKS16_DEF_DEP = ["16sinks.def"]
CTS_HELPERS_DEP = ["cts-helpers.tcl"]

Then you can use these variables to simplify the dictionary.

Comment on lines +99 to +246
"block_sta1": [
"example1_typ.lib",
"example1.def",
"example1.lef",
],
"find_clks1": [
"pad.lef",
"pad.lib",
],
"get_ports1_hier": [
"get_ports1.v",
],
"hier2": [
"example1_typ.lib",
"example1.lef",
"hier2_out.vok",
],
"hier_deep": [
"example1_typ.lib",
"example1.lef",
],
"hierclock": [
"hierclock_gate.v",
"hierclock_out.vok",
],
"network_edit1": [
"reg3.def",
],
"read_vcd": glob(
[
"Element.*",
"MockArray.*",
"asap7/*",
],
),
"read_verilog1": [
"reg1.v",
],
"read_verilog10": [
"hier1.v",
],
"read_verilog10_no_prop": [
"hier1.v",
],
"read_verilog11": [
"hier1.v",
],
"read_verilog2": [
"hier1.v",
],
"read_verilog3": [
"bus1.lef",
"bus1.lib",
"bus1.v",
],
"read_verilog5": [
"bidir1.lef",
"bidir1.lib",
],
"read_verilog9": [
"reg1.v",
],
"readdb_hier": [
"hier1.v",
],
"report_cell_usage_file": [
"report_cell_usage.def",
],
"report_cell_usage_modinsts": [
"hier1.v",
],
"report_cell_usage_modinsts_metrics": [
"hier1.v",
],
"report_cell_usage_physical_only": [
"report_cell_usage_no_taps.def",
],
"report_json1": [
"reg6.def",
],
"report_timing_histogram": [
"example1_slow.lib",
"example1.def",
"example1.lef",
],
"sdc_get1": [
"example1_slow.lib",
"example1.def",
"example1.lef",
],
"sdc_names1": [
"hier1.def",
],
"sta1": [
"example1_slow.lib",
"example1.def",
"example1.lef",
],
"sta2": [
"example1_slow.lib",
"example1.def",
"example1.lef",
"example1.sdf",
"example1.v",
],
"sta3": [
"example1_slow.lib",
"example1_fast.lib",
"example1.def",
"example1.lef",
],
"sta4": [
"example1_slow.lib",
"example1.dspef",
"example1.def",
"example1.lef",
],
"sta5": [
"example1_fast.lib",
"example1_slow.lib",
"example1_typ.lib",
"example1.def",
"example1.lef",
],
"write_sdc1": [
"hier1.sdc",
"hier1.v",
],
"write_verilog1": [
"reg1.def",
],
"write_verilog2": [
"reg4.def",
],
"write_verilog3": [
"bus1.def",
"bus1.lef",
"bus1.lib",
],
"write_verilog6": [
"write_verilog5.def",
],
"write_verilog8": [
"reg5.v",
],
"write_verilog9_hier": [
"write_verilog9.v",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This dictionary contains a lot of duplicated dependency lists and subset relationships. To improve readability and maintainability, consider defining variables for common dependencies before the list comprehension at line 92.

For example:

HIER1_V_DEP = ["hier1.v"]
EXAMPLE1_BASE_DEPS = ["example1.def", "example1.lef"]
STA1_DEPS = ["example1_slow.lib"] + EXAMPLE1_BASE_DEPS
BUS1_DEPS = [
    "bus1.lef",
    "bus1.lib",
]

Then you can reuse these variables in the dictionary, which will make it much cleaner.

Comment on lines +48 to +53
"scan_architect_clock_mix_sky130": [
"scan_architect_sky130.v",
],
"scan_architect_no_mix_sky130": [
"scan_architect_sky130.v",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To avoid duplication, you can define a variable for the dependency shared by scan_architect_clock_mix_sky130 and scan_architect_no_mix_sky130.

Define this before the list comprehension at line 41:

SCAN_ARCHITECT_SKY130_V_DEP = ["scan_architect_sky130.v"]

Then use it in the dictionary.

            "scan_architect_clock_mix_sky130": SCAN_ARCHITECT_SKY130_V_DEP,
            "scan_architect_no_mix_sky130": SCAN_ARCHITECT_SKY130_V_DEP,

Comment on lines +70 to +224
"boundary_push1": [
"testcases/boundary_push1.def",
"testcases/orientation_improve1.lef",
],
"boundary_push2": [
"testcases/boundary_push2.def",
"testcases/orientation_improve1.lef",
],
"boundary_push3": [
"testcases/boundary_push2.def",
"testcases/orientation_improve1.lef",
],
"centralization1": [
"testcases/centralization1.def",
"testcases/orientation_improve1.lef",
],
"clocked_macro": [
"testcases/clocked_macro.def",
"testcases/clocked_macro.lef",
"testcases/clocked_macro.lib",
"testcases/clocked_macro.v",
],
"fixed_covers": [
"Nangate45_io/dummy_pads.lef",
"testcases/fixed_macros1.def",
"testcases/orientation_improve1.lef",
],
"fixed_ios1": [
"Nangate45/Nangate45_fast.lib",
"testcases/fixed_ios1.def",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
],
"fixed_macros1": [
"testcases/fixed_macros1.def",
"testcases/orientation_improve1.lef",
],
"fixed_macros2": [
"testcases/fixed_macros2.def",
"testcases/orientation_improve1.lef",
],
"guides1": [
"testcases/guides1.def",
"testcases/guides1.v",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
],
"guides2": [
"testcases/macro_only.def",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
"testcases/macro_only.v",
],
"io_constraints1": [
"testcases/io_constraints1.def",
"testcases/io_constraints1.v",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
],
"io_constraints10": [
"testcases/io_constraints1.def",
"testcases/io_constraints1.v",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
],
"io_constraints2": [
"testcases/io_constraints1.def",
"testcases/io_constraints1.v",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
],
"io_constraints3": [
"testcases/io_constraints1.def",
"testcases/io_constraints1.v",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
],
"io_constraints4": [
"testcases/io_constraints1.def",
"testcases/io_constraints1.v",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
],
"io_constraints5": [
"testcases/io_constraints1.def",
"testcases/io_constraints1.v",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
],
"io_constraints6": [
"Nangate45/Nangate45_fast.lib",
"testcases/io_constraints6.def",
"testcases/macro_only.lef",
],
"io_constraints7": [
"Nangate45/Nangate45_fast.lib",
"testcases/io_constraints6.def",
"testcases/macro_only.lef",
],
"io_constraints8": [
"Nangate45/Nangate45_fast.lib",
"testcases/io_constraints6.def",
"testcases/macro_only.lef",
],
"io_constraints9": [
"testcases/io_constraints1.def",
"testcases/io_constraints1.v",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
],
"io_pads1": [
"Nangate45_io/dummy_pads.lef",
"testcases/io_pads1.def",
"testcases/macro_only.lef",
],
"keep_clustering_data": [
"Nangate45/Nangate45_fast.lib",
"testcases/io_constraints6.def",
"testcases/macro_only.lef",
],
"macro_only": [
"testcases/macro_only.def",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
"testcases/macro_only.v",
],
"macros_without_pins1": [
"Nangate45_io/dummy_pads.lef",
"testcases/macros_without_pins1.def",
"testcases/orientation_improve1.lef",
],
"mixed_ios1": [
"Nangate45/Nangate45_fast.lib",
"testcases/macro_only.lef",
"testcases/macro_only.lib",
"testcases/mixed_ios1.def",
],
"no_unfixed_macros": [
"testcases/macro_only.lef",
"testcases/macro_only.lib",
"testcases/macro_only.v",
"testcases/no_unfixed_macros.def",
],
"orientation_improve1": [
"testcases/orientation_improve1.def",
"testcases/orientation_improve1.lef",
],
"orientation_improve2": [
"testcases/orientation_improve1.def",
"testcases/orientation_improve1.lef",
],
"orientation_improve3": [
"testcases/orientation_improve1.def",
"testcases/orientation_improve1.lef",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This dictionary has a lot of duplicated dependency lists. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, you can define variables for these common lists before the list comprehension at line 64.

For example:

IO_CONSTRAINTS1_BASE_DEPS = [
    "testcases/io_constraints1.def",
    "testcases/io_constraints1.v",
    "testcases/macro_only.lef",
    "testcases/macro_only.lib",
]
IO_CONSTRAINTS6_BASE_DEPS = [
    "Nangate45/Nangate45_fast.lib",
    "testcases/io_constraints6.def",
    "testcases/macro_only.lef",
]
ORIENTATION_IMPROVE1_DEPS = [
    "testcases/orientation_improve1.def",
    "testcases/orientation_improve1.lef",
]
MACRO_ONLY_DEPS = [
    "testcases/macro_only.def",
    "testcases/macro_only.lef",
    "testcases/macro_only.lib",
    "testcases/macro_only.v",
]

Then you can use these variables to simplify the dictionary.

@github-actions
Copy link
Contributor

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

@maliberty
Copy link
Member

The gemini comments are reasonable but I'll leave refactoring to later

@maliberty maliberty merged commit dfd72d8 into The-OpenROAD-Project:master Jan 14, 2026
13 checks passed
@maliberty maliberty deleted the bzl-narrow-data branch January 14, 2026 20:59
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.

2 participants