Drop chisel

[oharboe]

I asked claude to ditch chisel and use system verilog for test/orfs/mock-array....

Needs review...

[github-actions]

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

[gemini-code-assist]

Code Review

This pull request replaces the Chisel-based MockArray implementation with a SystemVerilog version, removing Scala and Chisel dependencies and updating the Bazel build and OpenROAD flow scripts. Feedback identifies a critical syntax error in MockArray.sv caused by duplicated generate block labels, which necessitates updates to hierarchical paths in the TCL scripts. Furthermore, the glob patterns for pin matching in the TCL scripts likely require adjustment to correctly match packed and unpacked array indices, and an unused reset input was found in the MockArray module.

[gemini-code-assist]

The hierarchical path ces\[${r}\].ces\[${c}\].ces is based on an invalid SystemVerilog structure in MockArray.sv where generate block labels are duplicated. Once the generate block labels in MockArray.sv are fixed to be unique, this path will need to be updated to match the new valid hierarchy.

[gemini-code-assist]

The reset input to MockArray is declared but never used within the module. The rst inputs to the underlying Multiplier instances are all tied to 1'b0. If a reset is not necessary for this design, this input should be removed from the module's port list to improve clarity and avoid confusion.

  input  logic                  clock,

[oharboe]

well.... this is a test and interfaces can be fixed and inplementations may or may not use reset...

[maliberty]

Looks like gemini has some thoughts to address before I look.

[oharboe]

out of sight, out of mind for now. I'm good with this change, but my focus is elsewhere right now, so I don't want to think about nursing it through.

[github-actions]

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

[github-actions]

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

[oharboe]

/gemini review

[gemini-code-assist]

Code Review

This pull request replaces the Chisel-based MockArray design with a native SystemVerilog implementation and updates the Bazel build flow to support hierarchical synthesis. Key changes include removing Scala/Chisel dependencies, adding MockArray.sv, and refactoring mock-array.bzl to manage netlist stitching. Feedback identifies potential duplicate module definition errors during synthesis; it is recommended to guard the multiplier stub and Element wrapper in MockArray.sv with preprocessor macros and update the build configuration to define these macros during top-level synthesis.

[gemini-code-assist]

The multiplier stub module will cause duplicate module definition errors during hierarchical synthesis because it is also provided by multiplier.v. Since the build flow in mock-array.bzl concatenates the netlists, this stub should be guarded with `ifndef SYNTHESIS to avoid conflicts in the final stitched Verilog.

References

1. Rely on the toolchain to handle blackboxes instead of providing stubs that conflict with actual implementations during netlist stitching.

[gemini-code-assist]

The Element wrapper module is defined in the same file as MockArray. During hierarchical synthesis of MockArray, this definition will be included in the output netlist even if blackboxed, leading to duplicate module errors when stitched with the actual Element netlist. Consider guarding this definition with a macro (e.g., `ifndef BLACKBOX_ELEMENT) so it can be disabled when synthesizing the top-level array.

References

1. Avoid duplicate module definitions in concatenated netlists by guarding sub-module wrappers during top-level synthesis.

[gemini-code-assist]

To support the suggested guarding of the Element module in MockArray.sv during hierarchical synthesis, the BLACKBOX_ELEMENT macro should be defined here for the MockArray synthesis run.

References

1. Define tunable parameters and control macros as named constants or arguments instead of relying on defaults.

[oharboe]

Claude Response to Gemini review

Thanks for the review. Status of each comment:

✅ Fixed (commit 0fd71c20a5)

• Critical — duplicate generate-block label ces in MockArray.sv. Inner/outer loops now use ces_row / ces_col.
• Critical — load_mock_array.tcl hierarchical path updated to ces_row[${r}].ces_col[${c}].ces.

❌ Not applicable — Tcl regex, not glob

• High — element-io.tcl io_lsbIns.* pattern.
• High — io.tcl .* patterns on unpacked array ports.

match_pins in test/orfs/mock-array/util.tcl:38-56 uses Tcl regexp, not string match:

foreach pin [get_ports -regex .*] {
  set input [get_property $pin name]
  # We want the Tcl regex
  if { ![regexp $regex $input] } {
    continue
  }
  ...
}

In Tcl regex, . matches any character (including [), so io_lsbIns.* correctly matches io_lsbIns[0], io_lsbIns[1], …, and io_ins_down.* matches io_ins_down[0][0], etc. CI confirms I/O placement works. No change needed.

🚫 Rejected — covered in inline reply

• Medium — unused reset on MockArray. Already replied: this is a test, interfaces can be fixed and implementations may or may not use reset.

❌ Not applicable — Yosys already strips blackboxes

• High — multiplier stub should be `ifndef SYNTHESIS-guarded.
• High — Element wrapper should be `ifndef BLACKBOX_ELEMENT-guarded.
• Medium — define BLACKBOX_ELEMENT in mock-array.bzl.

Verified locally by inspecting the synth output (1_2_yosys.v):

$ grep '^module ' bazel-bin/.../MockArray/4x4_netlist/1_2_yosys.v
module MockArray(reset, clock, io_ins_left, ...);

$ grep '^module ' bazel-bin/.../Element/4x4_slang/1_2_yosys.v
module Element(clock, io_ins_left, ...);

Each slang-synth output contains exactly one module definition. Yosys's write_verilog strips modules marked blackbox via SYNTH_BLACKBOXES, so the concatenated netlist (MockArray netlist + Element netlist + multiplier netlist) has no duplicates. There is no error — CI passes.

I also tried the proposed guards locally. Both variants break synthesis:

• -DBLACKBOX_ELEMENT on the MockArray synth:

  ERROR: Module `\Element' referenced in module `\MockArray' in cell
  `\ces_row[3].ces_col[3].ces' is not part of the design.
  

• -DBLACKBOX_MULTIPLIER on the Element synth:

  ERROR: Module `\multiplier' referenced in module
  `\Multiplier$Element.inner.mult_left' in cell `\mult_inst' is not part of the design.
  

slang's --ignore-unknown-modules --empty-blackboxes silence the parse-time warning but don't register a blackbox in Yosys's design. The stub/wrapper definitions are required during slang elaboration so Yosys can derive port lists; the SYNTH_BLACKBOXES pass then strips the bodies before write_verilog. Adding the guards would regress a working flow.

Result

No new commit is warranted. All actionable critical issues were resolved in 0fd71c20a5; the remaining suggestions are based on an incorrect tooling model (Tcl regex vs glob, and Yosys blackbox semantics).

[oharboe]

@maliberty as long as this passes, I think we might be better off merging it and cleaning it up more later?

At least SystemVerilog means more people will touch it with a ten foot pole now...?

[oharboe]

@maliberty I dont think it is necessarily a bad thing that the test is a bit fuzzed(different).mock array tests are either very effective pre merge or there is little breakage in this area these days...

[oharboe]

something is wrong...

[github-actions]

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

[github-actions]

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

[oharboe]

Looks sane now:

bazelisk run --//:platform=gui test/orfs/mock-array:MockArray_8x8_base_final gui_final

[github-actions]

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

[github-actions]

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

[oharboe]

@maliberty I just merged w master and pr-head works. CI problem?