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 DataMirror.modulePorts #901

Merged
merged 3 commits into from
Oct 3, 2018
Merged

Add DataMirror.modulePorts #901

merged 3 commits into from
Oct 3, 2018

Conversation

ducky64
Copy link
Contributor

@ducky64 ducky64 commented Oct 3, 2018

Adds modulePorts(module: BaseModule): Map[String, Data] to experimental.DataMirror. This provides the necessary interfaces to split out Testers2 development (#866) into its own repo.

Points which may or may not warrant discussion:

  • This returns things as an unordered Map. Are there ever cases where we want to preserve ordering? Alternatives would be either a ListMap or a Seq.
  • BlackBox has its io contents flattened out in the return here, consistent with the generated component. Note that using io directly on a BlackBox can be illegal in some cases, because the top-level object actually does not get emitted (as an artifact of dropping the io_ prefix in the pre-ExtModule/MultiIOModule dats). The only alternative would be to error out if a BlackBox is passed in.

Related issue: N/A

Type of change: other enhancement

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes
Add chisel3.experimental.DataMirror.modulePorts(module: BaseModule): Map[String, Data]

@ducky64 ducky64 requested review from chick and jackkoenig October 3, 2018 01:09
@ducky64 ducky64 requested a review from a team as a code owner October 3, 2018 01:09
@ducky64
Copy link
Contributor Author

ducky64 commented Oct 3, 2018

Also, strictly speaking, DataMirror might not be the best place to put this, this might be more like Driver or DriverUtils, since the main purpose is to enable interaction with the compiled artifacts like simulators, instead of elaboration-time reflection.

@edwardcwang
Copy link
Contributor

This returns things as an unordered Map. Are there ever cases where we want to preserve ordering? Alternatives would be either a ListMap or a Seq.

How does Chisel preserve port ordering in ports/IOs of a module right now? e.g. the order of io_z vs io_a is preserved in the emitted FIRRTL.

val io = IO(new Bundle {
  val z = Output(Bool())
  val a = Output(Bool())
})

vs.

val io = IO(new Bundle {
  val a = Output(Bool())
  val z = Output(Bool())
})

@ducky64
Copy link
Contributor Author

ducky64 commented Oct 3, 2018

Module._ports is an ArrayBuffer, and Bundle.elements is a ListMap. So in theory the ordering is available, but I don't remember off the top of my head if the FIRRTL emitter takes it into account. Especially since ListMap stores in reversed order.

@edwardcwang
Copy link
Contributor

I mean (correct me if something has changed) the FIRRTL emitter and FIRRTL's Verilog emitter both seem to be respecting order in at least Bundles right now, no?

Copy link
Contributor

@chick chick left a comment

Choose a reason for hiding this comment

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

This looks good.

Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

Order is preserved through to the emitted Verilog so I really think we should preserve it here

@ducky64
Copy link
Contributor Author

ducky64 commented Oct 3, 2018

So it appears there is not a Scala collection type that preserves ordering (ListMap stores things in reversed order). SeqMap/VectorMap is apparently slated for Scala 2.13, so that might be a future migration path. But for now, do we want this to be a Seq or a Map?

@edwardcwang
Copy link
Contributor

We could provide the map as an additional function e.g. .modulePortsMap if we wanted to?

@ducky64
Copy link
Contributor Author

ducky64 commented Oct 3, 2018

Ok, I'm going to change the API to a Seq return, and since this is experimental we can change the API to VectorMap/SeqMap in the future. Or move this into like DriverUtils. Both of which I'll write into the comments so we don't forget about it.

@ducky64
Copy link
Contributor Author

ducky64 commented Oct 3, 2018

And with a Seq API, if users want a map they can always do .toMap. I'm generally not in favor of presenting redundant APIs vs. a composition approach in calling code.

@ducky64
Copy link
Contributor Author

ducky64 commented Oct 3, 2018

Changed the interface to be Seq[(String, Data)]. Added TODOs for possibly moving from DataMirror to Driver or something, and for changing the return type to SeqMap when it becomes available. Reverses the BlackBox ports order, to make them consistent with the declared order, since ListMaps store elements in reverse.

@davidbiancolin
Copy link
Contributor

davidbiancolin commented Oct 3, 2018

This looks good. I should note, in MIDAS we call UserModule.getPorts to get a firrtl port list, from which we reconstruct the module's IO. There's a comment above that member :

// For debuggers/testers, TODO: refactor out into proper public API

+1 For order preserving.

Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

I would argue that ListMap does preserve order, reversed is still ordered :)

Anyway, I'm down with Seq.

@ducky64 ducky64 merged commit dafdeab into master Oct 3, 2018
@ducky64 ducky64 deleted the getPorts branch October 3, 2018 23:15
@ucbjrl ucbjrl added this to the 3.2.0 milestone Dec 4, 2018
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.

6 participants