Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Logic.named and broaden clone #550

Merged
merged 5 commits into from
Jan 21, 2025
Merged

Conversation

mkorbel1
Copy link
Contributor

Description & Motivation

The main things in this PR are:

  • adding Logic.clone, which behaves similarly to the existing ones for LogicStructure
  • adding Logic.named, which automates the creation of a named clone of a signal (usually for the purpose of making generated outputs prettier)

Related Issue(s)

N/A

Testing

Added new tests

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

No

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

Not really, API docs cover it, maybe some general examples and guidance would be nice in the future

Copy link
Contributor

@ganewto ganewto left a comment

Choose a reason for hiding this comment

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

These changes look good and the naming is effective, especially with the merging options.

  • you may want to reference the Naming option in .named description so people check that out.
  • Logic(name: 'my_important_signal').named('my_really_important_signal'); makes it so we can rename signals, but what is really happening is we are getting a new Logic? We should clarify.
  • We have a slight inconsistency with instances now where we cannot do a .named() on a module instantiation similar to above.
  • Is there any way to pickup the name of the Dart variable. If so, we can use decent names for Dart variables and get them for free in the Verilog (and guaranteed to match). Then we would only used .named() for the merging options or if we have a Verilog conflict or want a different naming convention in Verilog than in Dart. I find I only use the merging options in detailed components, but not in larger blocks where the names are generally unique.

@mkorbel1
Copy link
Contributor Author

  • you may want to reference the Naming option in .named description so people check that out.
  • Logic(name: 'my_important_signal').named('my_really_important_signal'); makes it so we can rename signals, but what is really happening is we are getting a new Logic? We should clarify.

I think the doc comment covers these?

  /// Makes a [clone] with the provided [name] and optionally [naming], then
  /// assigns it to be driven by `this`.
  ///
  /// This is a useful utility for naming the result of some hardware
  /// construction without separately declaring a new named signal and then
  /// assigning.  For example:
  • We have a slight inconsistency with instances now where we cannot do a .named() on a module instantiation similar to above.

I don't think it makes sense to have a named on a Module instance, since it doesn't make sense to "copy and assign" like we can for Logics. Instance names of modules have to be defined per-module. We could perhaps add some method for overriding instance names, but I'm not sure that's necessary or sufficiently useful?

  • Is there any way to pickup the name of the Dart variable. If so, we can use decent names for Dart variables and get them for free in the Verilog (and guaranteed to match). Then we would only used .named() for the merging options or if we have a Verilog conflict or want a different naming convention in Verilog than in Dart. I find I only use the merging options in detailed components, but not in larger blocks where the names are generally unique.

This would bring reflection into ROHD, which implies the structure and naming of the generator software has an impact on the generated hardware (or at least its naming). Philosophically, this has been a strength of ROHD vs. other new HDL approaches like Chisel. I think the Dart macros feature will provide a good mechanism in the future for saving on redundancy for naming things identically without forcing reflection more broadly.

@mkorbel1 mkorbel1 merged commit f26de26 into intel:main Jan 21, 2025
3 checks passed
@mkorbel1 mkorbel1 deleted the namedclones branch January 21, 2025 18:37
@mkorbel1 mkorbel1 linked an issue Jan 23, 2025 that may be closed by this pull request
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.

Add an easier way to name important generated signals
2 participants