Module services api - #673
Conversation
…al/instance naming routine names
Introduces a singleton service registry (ModuleServices) that provides a unified query surface for DevTools and inspection tools. Module.build() now registers the root module with ModuleServices.instance. Also adds SvService which wraps SystemVerilog synthesis and registers with ModuleServices for DevTools access to SV metadata. This is a clean separation: no netlist code is included. The netlist branch will later extend ModuleServices with a netlistService field.
| /// - [outputPath] — the default file or directory written by [write]. | ||
| /// - [multiFile] — whether [write] emits one file per module definition | ||
| /// (a directory) or a single combined file. | ||
| /// - [write] — performs the write, honouring [multiFile]. |
There was a problem hiding this comment.
i like this API because it's easy to understand, but should we be using some sort of URI object or something from Dart instead that supports referencing files, directories, and other weird stuff instead? im not sure the answer, genuine question
There was a problem hiding this comment.
if we don't fix before next release, will this not get us stuck with the API?
Backed by _rootModule. Setter eagerly syncs ModuleTree.rootModuleInstance. Safe for module_services_api (no FLC dependencies).
| /// // Or get the concatenated output (like generateSynth): | ||
| /// print(sv.allContents); | ||
| /// ``` | ||
| class SystemVerilogService extends CodeGenService { |
There was a problem hiding this comment.
Something doesn't feel quite right to me in this API. Some thoughts:
- If I want to generate some SV (pretty common), why am I constructing a "service"? Though the abstraction levels as a service make sense to me.
- Why does the act of constructing said service immediately produce the output?
- In what cases do I want to run the SynthBuilder with a selected synthesizer instead of this service?
- When there are multiple synthesizers (e.g. SystemC), will I have multiple services or one service that takes multiple synthesizers?
The two APIs currently are basically:
- Turn this module into SV quickly (generateSynth function on module)
- Make a thing that builds an output based on this Synthesizer and give me outputs I can do more work with (SynthBuilder with Synthesizer arg)
This is deprecating the first, but keeping the second, and there's still some API overlap.
I think this is close to right, but something is a little off. Thoughts?
There was a problem hiding this comment.
Let me ponder this.
I have NetlistService, SystemVerilogService, SystemCService, TraceService, and WaveformService. So some of this is trying to go for least surprise even though they are not fully symmetric. For example, the outputing services need to be setup after the build. WaveformService tracks with the simulator running, so it is truly a live 'service', but the output services execute (though we could use an api that says 'generate' for those.
I never thought of SynthBuilder as a user API.
Here is a live usage on HCL:
final adder = FloatingPointAdderSinglePath(clk: clk, fp1, fp2);
await adder.build();
NetlistService(adder,
outputPath: '$buildDir/${adder.definitionName}.json');
final sv =
SystemVerilogService(adder, outputPath: '$buildDir/${adder.definitionName}.sv'
// embedSourceTraceComments: false,
);
final sc = SystemCService(adder,
outputPath: '$buildDir/${adder.definitionName}.sc', multiFile: false);
TraceService(adder, svService: sv, scService: sc).write('x.flc.json');
WaveformService(adder,
format: WaveOutputFormat.fst,
outputPath: '$buildDir/${adder.definitionName}.fst');There was a problem hiding this comment.
One additional thing: the synthesizers must run to store FLC info. Then the TraceService can access to generate FLC.
There was a problem hiding this comment.
I think of these services as constructing an in-memory view that can be further manipulated (written to file, written to string, perhaps searched, etc). The only challenge I face is that if we change any line numbers then trace gets broken.
There was a problem hiding this comment.
A added simpler Module.dumpSystemVerilog and Module.dumpWaveforms which don't require forming a Service.
I discovered some usefuless to the Service model: it can stay alive and provide an API, so for example, the NetlistService can provide connectivity queries beyond just netlisting as it has created a model of the design. We could do that with Module after build, but there are other examples like the WaveformService enabling incremental waveform queries. We can also make these more highly optioned so we don't keep extending all new capabilities as part of Module.
There was a problem hiding this comment.
i dont see these in the current PR? also, does dumpWaves work better?
|
|
||
| # run tests in JS (increase heap size also) | ||
| # Run tests in sub-packages | ||
| for pkg in packages/*/; do |
There was a problem hiding this comment.
what about hierarchical packages (like rohd_devtools_widgets)?
There was a problem hiding this comment.
See comment below. pub workspace is the Dart/Flutter answer.
| /// // Or get the concatenated output (like generateSynth): | ||
| /// print(sv.allContents); | ||
| /// ``` | ||
| class SystemVerilogService extends CodeGenService { |
There was a problem hiding this comment.
i dont see these in the current PR? also, does dumpWaves work better?
| /// | ||
| /// For more control over filtering, timescale, and recording windows, see | ||
| /// [WaveformService] constructor parameters. | ||
| @Deprecated('Use WaveformService instead') |
There was a problem hiding this comment.
or the dumpWaves on Module?
There was a problem hiding this comment.
Yes, the deprecation mentions first using Module.dumpWaves() and then the WaveformService.
| /// | ||
| /// For more control over filtering, timescale, and recording windows, see | ||
| /// [WaveformService] constructor parameters. | ||
| @Deprecated('Use WaveformService instead') |
There was a problem hiding this comment.
Update documentation (doc/ directory) as well.
| final dut = ManySubmodulesModule(Logic(), numSubModules: 10000); | ||
| await dut.build(); | ||
| dut.generateSynth(); | ||
| SystemVerilogService(dut).output; |
There was a problem hiding this comment.
should use dumpSystemVerilog instead in most cases, including tutorials, benchmarks, simple tests, etc?
| await _mod.build(); | ||
|
|
||
| WaveDumper(_mod, outputPath: _vcdTemporaryPath); | ||
| WaveformService(_mod, outputPath: _vcdTemporaryPath); |
There was a problem hiding this comment.
should use dumpWaves in most scenarios including simple tests, benchmarks, tutorials, etc?
There was a problem hiding this comment.
Done. Renamed to dumpWaves. Using the two dump routines everywhere except the new services tests.
| /// DevTools evaluates this via `EvalOnDartLibrary` to display the module | ||
| /// hierarchy. Richer design views (e.g. a slim netlist) are composed by the | ||
| /// DevTools client from the relevant registered service. | ||
| String get hierarchyJSON => ModuleTree.instance.hierarchyJSON; |
There was a problem hiding this comment.
| /// filtering, timescale, start/stop times, flush size, and overwrite policy. | ||
| WaveformService( | ||
| this.module, { | ||
| this.outputPath = 'waves.vcd', |
There was a problem hiding this comment.
should this default to null and automatically pick the name based on format instead? e.g. if someone picks format: .fst but doesnt specify the path, it would be weird for it to be waves.vcd
maybe if format is default to null also then you can infer the type from the file name too? e.g. if someone says waves.fst it defaults format to fst?
then just need to document behavior if there is none specified for either?
There was a problem hiding this comment.
I reworked these substantially to consider the idea of having byte stream as well as file output, and defaults like module name and use the format as the extension, etc. So it would be good to reanalyze the changes to the API. Conceptually similar, but more scalable.
| /// | ||
| /// Requires an FST writer to be available; see the DevTools subclass for | ||
| /// a fully FST-backed implementation. | ||
| fst, |
There was a problem hiding this comment.
is FST supported in this PR?
There was a problem hiding this comment.
No, but we check here and throw. We should add fst soon.
| dart pub get | ||
|
|
||
| # Install dependencies for sub-packages | ||
| for pkg in packages/*/; do |
There was a problem hiding this comment.
what about sub-sub-packages?
There was a problem hiding this comment.
The suggestion is to use a pub workspace. Since there is a mix of flutter and dart packages, this means flutter controls. But that gives us full repo analysis and testing without scripting, but with a dart program to manage.
There was a problem hiding this comment.
There was a problem hiding this comment.
…ices_api # Conflicts: # doc/tutorials/chapter_3/answers/exercise_sv.dart
Description & Motivation
This is a subset of the PR #669 that introduces the new API for Services attached to a module (outputters like wave dumpers, netlisters, translators, tracing services, etc).
Related Issue(s)
None
Testing
This runs a full dart test with the new API in place in the tests.
Backwards-compatibility
No, we retain the old interfaces but they should eventually be deprecated.
WaveDumper
generateSynth
...
Documentation
Some examples have been modified to match.