Repository navigation
Typed operations - #704
Typed operations#704
Conversation
80074ea to
4edea94
Compare
mkorbel1
left a comment
mkorbel1
left a comment
There was a problem hiding this comment.
Keeping structures and arrays intact through operations would remove a lot of reconstruction at call sites. I think we should build that capability into the existing APIs and shared machinery, rather than introduce parallel typed operation and pipeline families. The comments below suggest that direction and raise a few choices for us to work through, including output construction and whether a small, explicit compatibility break for constant inputs is worthwhile.
These proposed API upgrades would introduce breaking changes. For example, existing calls that flop a plain Const could need an explicit <Logic> type argument. If we accept that direction, we'd need to publish it as ROHD 0.7.0, rather than another 0.6.x release, and include migration notes for the affected calls. That release implication is part of the tradeoff for us to agree on, not just an implementation detail.
I've focused this pass on architecture and API design in the typed-operation increment, rather than re-reviewing the older #686 snapshot included in this branch. Bringing in the current #686 work would give us the integrated design for the next pass. Once we've worked through these decisions, we can follow up with the detailed correctness review.
Verification so far is limited: six focused existing tests passed at the reviewed head, including Icarus build-only checks for the structured-constant mux, flop, and passthrough. That is not a full-suite result or complete verification of constant handling and the packed/unpacked/mixed-array emission paths discussed below. Constants need thorough coverage across the affected typed APIs; the existing passing cases do not establish that coverage.
One nonblocking note: there is some formatting-only churn mixed into the change. It may be fine here; the main thing is that we avoid changes that the normal formatter would immediately reverse. We can use AI-assisted comparison to separate that noise from the semantic changes during review. The 27 incremental library files passed the non-writing formatter check under Dart 3.13.0, so I haven't found a formatter-stability problem.
f9c35ae to
af0c8d5
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Typed-array operations can silently reinterpret values through incompatible codecs, and the compatibility claims and documentation contain inaccuracies.
4 open findings
What changed in this PR
Introduces type-preserving operations for structured signals and extends synthesis support for typed arrays.
Changes:
- Makes muxes, flops, cases, selection, pipelines, and passthroughs preserve concrete
Logictypes. - Adds typed array/value handling across SystemVerilog and netlist synthesis.
- Updates migration documentation, removes deprecated APIs, and expands tests.
| File | Description |
|---|---|
.devcontainer/devcontainer.json |
Adds Node.js 24. |
.github/configs/mlc_config.json |
Updates link-check exclusions. |
CHANGELOG.md |
Documents 0.7.0 changes. |
benchmark/ssa_driver_search_benchmark.dart |
Updates pipeline API usage. |
doc/logic_enum_netlist_metadata.md |
Proposes enum netlist metadata. |
doc/logic_enum_typed_operations.md |
Plans enum operation integration. |
doc/user_guide/_docs/A19-logic-structures.md |
Documents typed structure operations. |
doc/user_guide/_docs/A20-logic-arrays.md |
Documents typed arrays and values. |
doc/user_guide/_docs/A21-generation.md |
Expands synthesis guidance. |
lib/rohd.dart |
Hides internal array base. |
lib/src/diagnostics/inspector_service.dart |
Removes legacy custom-module handling. |
lib/src/external.dart |
Removes deprecated alias. |
lib/src/finite_state_machine.dart |
Removes deprecated FSM APIs. |
lib/src/interfaces/interface.dart |
Preserves typed arrays in interfaces. |
lib/src/interfaces/pair_interface.dart |
Updates cloning and uniquification. |
lib/src/module.dart |
Extends typed port support. |
lib/src/modules/bus.dart |
Optimizes packed-array swizzles. |
lib/src/modules/conditionals/case.dart |
Adds typed cases. |
lib/src/modules/conditionals/combinational.dart |
Removes legacy custom-SV handling. |
lib/src/modules/conditionals/conditional.dart |
Removes deprecated accessors. |
lib/src/modules/conditionals/conditional_group.dart |
Tracks aggregate assignments. |
lib/src/modules/conditionals/flop.dart |
Adds generic typed flops. |
lib/src/modules/conditionals/if.dart |
Removes deprecated IfBlock. |
lib/src/modules/conditionals/sequential.dart |
Removes deprecated alias. |
lib/src/modules/gates.dart |
Adds generic typed muxes. |
lib/src/modules/modules.dart |
Exports passthrough publicly. |
lib/src/modules/operation_utils.dart |
Centralizes typed-operation validation. |
lib/src/modules/passthrough.dart |
Adds generic typed passthroughs. |
lib/src/modules/pipeline.dart |
Adds typed pipeline variants. |
lib/src/selection.dart |
Preserves selection result types. |
lib/src/signals/base_logic_array.dart |
Introduces shared array infrastructure. |
lib/src/signals/logic.dart |
Adds generic selection support. |
lib/src/signals/logic_array.dart |
Specializes typed array behavior. |
lib/src/signals/logic_structure.dart |
Adds typed cloning and compatibility. |
lib/src/signals/logic_structure_signature.dart |
Adds synthesis shape signatures. |
lib/src/signals/port.dart |
Removes deprecated Port. |
lib/src/signals/signals.dart |
Registers new array parts. |
lib/src/signals/typed_logic_array.dart |
Implements typed hardware arrays. |
lib/src/swizzle.dart |
Removes deprecated helpers. |
lib/src/synthesizers/netlist/netlist_cell_mapper.dart |
Maps passthrough primitives. |
lib/src/synthesizers/netlist/netlist_module_translation.dart |
Emits nested aggregate cells. |
lib/src/synthesizers/netlist/netlist_synth_module_definition.dart |
Supports typed array decomposition. |
lib/src/synthesizers/netlist/netlist_synthesis_result.dart |
Removes deprecated output API. |
lib/src/synthesizers/netlist/netlist_synthesizer.dart |
Extends aggregate alias handling. |
lib/src/synthesizers/netlist/netlist_synthesizer_configuration.dart |
Adds aggregate primitive defaults. |
lib/src/synthesizers/netlist/netlist_utils.dart |
Serializes typed array metadata. |
lib/src/synthesizers/netlist/netlist_validation.dart |
Validates aggregate drivers. |
lib/src/synthesizers/synth_builder.dart |
Removes deprecated builder APIs. |
lib/src/synthesizers/synthesis_result.dart |
Removes deprecated file conversion. |
lib/src/synthesizers/systemverilog/system_verilog_service.dart |
Marks compatibility API deprecated. |
lib/src/synthesizers/systemverilog/systemverilog_mixins.dart |
Removes legacy custom-SV mixin. |
lib/src/synthesizers/systemverilog/systemverilog_synth_module_definition.dart |
Lowers nested and packed arrays. |
lib/src/synthesizers/systemverilog/systemverilog_synthesis_result.dart |
Updates declarations and ranges. |
lib/src/synthesizers/systemverilog/systemverilog_synthesizer.dart |
Removes legacy synthesis paths. |
lib/src/synthesizers/systemverilog/systemverilog_synthesizer_configuration.dart |
Adds Icarus workaround. |
lib/src/synthesizers/utilities/synth_array_concat.dart |
Generalizes array concatenation. |
lib/src/synthesizers/utilities/synth_array_slice.dart |
Generalizes array slicing. |
lib/src/synthesizers/utilities/synth_logic.dart |
Supports typed array references. |
lib/src/synthesizers/utilities/synth_module_definition.dart |
Updates aggregate assignments. |
lib/src/synthesizers/utilities/synth_module_stop_policy.dart |
Treats operations as primitives. |
lib/src/synthesizers/utilities/synth_structure_layout.dart |
Handles typed array boundaries. |
lib/src/utilities/config.dart |
Updates framework version. |
lib/src/utilities/simcompare.dart |
Supports typed-array vectors. |
lib/src/values/logic_value.dart |
Adds packed shaped-value behavior. |
lib/src/values/logic_value_array.dart |
Adds shaped logic values. |
lib/src/values/typed_logic_value_array.dart |
Adds codec-backed typed values. |
lib/src/values/values.dart |
Registers array value parts. |
lib/src/wave_dumper.dart |
Updates deprecation lifecycle. |
pubspec.yaml |
Bumps version to 0.7.0. |
rohd_devtools_extension/pubspec.yaml |
Constrains material_ui. |
test/array_collapsing_test.dart |
Updates direct assignment expectations. |
test/async_reset_test.dart |
Uses explicit scalar flops. |
test/logic_array_test.dart |
Tests empty and packed arrays. |
test/logic_name_config_test.dart |
Tests typed-array synthesis merging. |
test/logic_structure_test.dart |
Tests aggregate assignment and flattening. |
test/logic_value_array_test.dart |
Tests shaped logic values. |
test/long_chain_test.dart |
Updates public passthrough import. |
test/module_test.dart |
Uses explicit scalar muxing. |
test/multimodule3_test.dart |
Updates passthrough imports. |
test/multimodule4_test.dart |
Reformats traversal assertions. |
test/multimodule5_test.dart |
Uses generic passthrough. |
test/naming_cases_test.dart |
Tests nested typed-array names. |
test/net_bus_test.dart |
Updates optimized swizzle output. |
test/netlist_synthesizer_test.dart |
Tests aggregate cells and validation. |
test/pair_interface_hier_uniquify_test.dart |
Migrates hierarchical naming. |
test/pair_interface_test.dart |
Tests typed arrays in interfaces. |
test/pipeline_test.dart |
Migrates legacy signal pipeline. |
test/provider_consumer_uniquify_test.dart |
Migrates interface naming. |
test/shuffle_test.dart |
Updates imports and copyright. |
test/submodule_output_mapping_test.dart |
Removes legacy custom-SV coverage. |
test/subset_range_mapping_test.dart |
Tests net range references. |
test/sv_gen_test.dart |
Tests direct packed-array assignments. |
test/swizzle_test.dart |
Updates optimized swizzle expectation. |
test/synth_builder_test.dart |
Removes deprecated API coverage. |
test/synth_name_parity_test.dart |
Tests typed-array naming parity. |
test/systemverilog_port_types_test.dart |
Tests nested output declarations. |
test/typed_logic_array_inout_test.dart |
Tests typed structured inouts. |
test/typed_logic_array_test.dart |
Expands typed-array coverage. |
test/typed_logic_value_array_test.dart |
Tests typed value arrays. |
test/typed_operations_test.dart |
Tests type-preserving operations. |
test/typed_port_test.dart |
Tests constant structured inputs. |
test/waveform_service_test.dart |
Uses explicit scalar flop output. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
282e917 to
eeebe9b
Compare
mkorbel1
left a comment
mkorbel1
left a comment
There was a problem hiding this comment.
The unified generic operation APIs are a substantial improvement. Output construction is shared, the d0/generator/shortcut contracts are explicit, the unused TypedOp base is gone, and the clone/name extensions now cover Logic with private helpers. Those parts address much of the earlier feedback.
I found two correctness gaps in the integrated implementation: constant-reset enum flops crash during JSON netlist export, and aggregate operation validation accepts incompatible enum leaf mappings that scalar validation rejects. Both have small controlled reproductions.
The earlier pipeline/SSA, direct-emission, and shared-synthesis requests also remain open in the code despite the resolved threads. Please remove the duplicate pipeline families and preserve the existing Pipeline and ReadyValidPipeline APIs and names. Types belong to individual signal identifiers and accessor calls, not to the pipeline. The wrapper and synthesis ownership questions also need resolution. There is some additional public surface to trim or justify, including flattenOuter, which was removed during the array review, and the unnecessary output-generator API on Passthrough.
Local verification at this head passed 1,717 checked-in VM tests with one skip, and 234 Node tests with 17 skips. Six additional enum-array cases passed ROHD and Verilator simulation across packed, mixed, and unpacked representations, with enum generation on and off. Four other reviewer checks reproduce the two defects; static analysis, formatting, documentation, and same-head CI are clean. These are focused results, not an exhaustive suite claim.
The remaining #686 fixes are upstream dependencies, not new duplicate findings here. Once those land, this should need a focused integration pass, but the correctness and API/architecture items below still need to be resolved first.
0efe7b8 to
8928d01
Compare
Retain scalar output naming and mergeable cases results during typed operation migration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve internally computed sources for packed array outputs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Desmond A. Kirkpatrick <desmond.a.kirkpatrick@intel.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
faa3d5d to
1e62b45
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Will merge -- have covered review issues.
Description & Motivation
Our ROHD operations flatten attached structured signals,
LogicArrayandLogicStructuretoLogic, and we lose abstraction. This is especially noticeable when composing modules that share these structured types, and it shows up when we netlist or produce other outputs that could retain this structure.Really, a
Muxcan be interpreted as aMux<T>where the baseline case isMux<Logic>. We know the data input is the actual type, and so we can even infer the output type. Tricky cases exist when we pass inConst, so those need careful handling.Note that this also needs to handle the new
TypedLogicArraytype, so this PR is based on PR #686.This PR also has support for
ConstLogicStructureto support fixing ROHD-HCL Closes #200Closes #426
Closes #559
Closes #587
Closes #615
Related Issue(s)
Testing
All existing tests pass. Several new tests, including some changes to the netlister, help validate this approach.
Especially issue #559 which has specific tests to keep the fix narrow.
Backwards-compatibility
No.
Documentation
Yes. Changes were added to mentions of the operators and in the
LogicStructureandLogicArrayareas.