Define Data .toString #985

ducky64 · 2019-01-16T23:13:51Z

toString now defined on Data subtypes, and handles literals properly.

See test cases for expected behavior.

Meant to support testers2, which uses literals as its interface. So you can directly print the result of a peek, instead of unpacking it through litValue and friends. Also gives type information for printf debugging when writing RTL.

Discussion points:

Naming for Bundles and ChiselEnums are inconsistent, as ChiselEnum uses a qualified path name where Bundles are just the class simpleName. Perhaps we should standardize on one? @hngenc
@hngenc comments on the changes to StrongEnum.scala are welcome.
I'm not sure why Bundle has its className hardcoded to Bundle. I removed that to get it to default to the class name in Record. @jackkoenig appears to be the last person associated with that line of code, thoughts?
The FixedPoint parameter print style is consistent with FIRRTL <width><<binaryPoint>> but not how I've defined Vec <n, type>. Note that FIRRTL defines Vec as type[n]. @azidar thoughts if we want things to print one particular way or the other?
Some potential for deduplication with Emitter.scala.
We currently don't have source locators for binding operations, but it could be added in the future.

Related issue: Resolves #953

Type of change: other enhancement

Impact: no functional change

Development Phase: implementation

Release Notes
toString now defined on Data subtypes, and handles literals properly.

edwardcwang · 2019-01-16T23:38:00Z

General questions: is this meant to be the "standard definition/format" for printing Chisel objects? Is testers2 going to parse these strings?

jackkoenig · 2019-01-16T23:52:01Z

chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala

@@ -543,8 +559,6 @@ class AutoClonetypeException(message: String) extends ChiselException(message)
  * }}}
  */
 abstract class Bundle(implicit compileOptions: CompileOptions) extends Record {
-  override def className = "Bundle"
-


className is used by Printable for hardware pretty printing. I think just using the default Aggregate .getClass.getSimpleName is better for obvious reasons. I can't recall my original thinking, but I think it had to do with making this pretty printing look like printing a Map in Scala (since that's sort of what Records/Bundles are), but I think giving the name of the Bundle class is generally better.

My only question is what about anonymous or inner bundles? You'll get some pretty weird names but perhaps that's still desirable.

Since the intention of this is for human debugging, I think providing better naming on a best-effort basis makes sense. So anonymous Bundles will probably give some nasty name. Inner bundles should work fine, it's one of the test cases (though not intentionally).

Random thought: maybe we could regex match anonymous bundle names and replace it with a more friendly AnonymousBundle? Might be more trouble than it's worth though, so I'd prefer to wait for user feedback first.

Yeah I think that's sensible, at least MyModule$anon$1 tells you more than just Bundle!

ducky64 · 2019-01-17T00:26:10Z

This is meant to provide a human-readable format for debugging. It's not meant to be (and should not be) machine readable.

If there was some kind of standard, this should follow it to the extent the standard provides a human-readable format, but I'm not trying to define a standard here. The print representation can change as necessary.

chick

This looks pretty good (and useful) to me.

jackkoenig · 2019-01-17T18:10:11Z

src/test/scala/chiselTests/DataPrint.scala

+  }
+
+  class BoundDataModule extends MultiIOModule {  // not in the test to avoid anon naming suffixes
+    Wire(UInt()).toString should be("UInt(Wire in DataPrintSpec$BoundDataModule)")


Do you think it might be worth having a source locator for Data? Let people know where the Wire was constructed?

Possibly. This is new enough that I don't know what the use case for this will be (other than printing literals), so I'd like to wait and see first.

There would also need to be infrastructure built for it, mainly IO needs to have a source locator attached to it, and all Bindings needs a copy of the source locator (currently is only part of the FIRRTL command). Adding source locators for literals would be more difficult because of the possibility for a chained apply, so we might have to build more source info transform macros.

ducky64 · 2019-01-18T00:49:38Z

Another interesting idea would be to print IO names where possible, though naming data isn't available until after the Module finishes elaboration. Perhaps something like
Clock(IO in unelaborated MyModule)
and
Clock(IO clock in MyModule)

hngenc · 2019-01-18T23:23:31Z

chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala

+    val bindingString = litOption match {
+      case Some(value) => factory.nameOfValue(value) match {
+        case Some(name) => s"($value=$name)"
+        case None => s"($value)"


Would it be better to put something like ($value=INVALID) in the None case?

hngenc · 2019-01-18T23:31:19Z

I made a small comment, but overall, I think the changes to ChiselEnum are just fine.

@ducky64 If you're talking about changing enumTypeName to the simple name, then I'm fine with that.

ducky64 · 2019-01-19T00:25:29Z

I attempted to dedup w/ FIRRTL emit, but there's enough different functionality that I don't think it's worth it. In particular, FIRRTL doesn't have named Record / Bundles, but it doesn't make sense to print the entire Bundle definition.

Other changes:

Changed Vec print format to match FIRRTL
Changed bad Enum print format to ($value=(invalid)). Not sure how to test for that since I can't create an invalid Enum.
Added DataMirror.fullModulePorts as a utility function, that returns the underscore-delimited names for Bundle or Vec elements.
Added printing IO names for elaborated modules. They're underscore-delimited, so consistent with FIRRTL but not what would be expected from Chisel. Thoughts welcome.

significant changes, re-review requested

ducky64 · 2019-01-19T00:50:54Z

Added fallback Bundle naming so anonymous Bundles are named Bundle, otherwise they had an empty string

chick

I think this looks good and should be helpful

ducky64 · 2019-01-22T22:24:44Z

Looks like anonymous bundles return a different name on different platforms ($anon$2 vs empty string), this now supports both versions that I've seen.
Also changed the name output of anonymous bundles to AnonymousBundle, which better indicates that it should handle named bundles properly.

ducky64 requested a review from chick January 16, 2019 23:13

ducky64 requested a review from a team as a code owner January 16, 2019 23:13

jackkoenig reviewed Jan 16, 2019

View reviewed changes

chick previously approved these changes Jan 17, 2019

View reviewed changes

jackkoenig reviewed Jan 17, 2019

View reviewed changes

hngenc reviewed Jan 18, 2019

View reviewed changes

ducky64 force-pushed the dataPrints branch from 5868047 to 1f5a429 Compare January 19, 2019 00:31

ducky64 added 8 commits January 18, 2019 16:32

untested code for everything except strongenum

a872cdb

Tests and fixes

5f0fdae

more fixes

f474468

revert binding changes

c65fbfc

Update Binding.scala

e1efde0

Make it clear DontCare is bound

5ad66ec

fixes

b0eb007

Use SimpleName for enums

fc6a23e

ducky64 force-pushed the dataPrints branch from 1f5a429 to fc6a23e Compare January 19, 2019 00:32

Clean up StrongEnum toString, add fallback Bundle naming

c6a9519

style fix

40b5eef

chick approved these changes Jan 22, 2019

View reviewed changes

support other anonymous bundle getSimpleName style

38b1542

Merge branch 'master' into dataPrints

0469462

ducky64 merged commit b39bc25 into master Jan 22, 2019

ducky64 deleted the dataPrints branch January 22, 2019 23:06

edwardcwang mentioned this pull request Feb 24, 2019

Calling == in bindingToString can cause a weird crash #1027

Closed

jackkoenig mentioned this pull request Jun 13, 2019

Bad error message when using experimental.IO #1109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define Data .toString #985

Define Data .toString #985

ducky64 commented Jan 16, 2019

edwardcwang commented Jan 16, 2019

jackkoenig Jan 16, 2019

ducky64 Jan 17, 2019

jackkoenig Jan 17, 2019

ducky64 commented Jan 17, 2019

chick left a comment

jackkoenig Jan 17, 2019

ducky64 Jan 17, 2019

ducky64 commented Jan 18, 2019

hngenc Jan 18, 2019 •

edited

Loading

hngenc commented Jan 18, 2019

ducky64 commented Jan 19, 2019

ducky64 commented Jan 19, 2019

chick left a comment

ducky64 commented Jan 22, 2019

Define Data .toString #985

Define Data .toString #985

Conversation

ducky64 commented Jan 16, 2019

edwardcwang commented Jan 16, 2019

jackkoenig Jan 16, 2019

Choose a reason for hiding this comment

ducky64 Jan 17, 2019

Choose a reason for hiding this comment

jackkoenig Jan 17, 2019

Choose a reason for hiding this comment

ducky64 commented Jan 17, 2019

chick left a comment

Choose a reason for hiding this comment

jackkoenig Jan 17, 2019

Choose a reason for hiding this comment

ducky64 Jan 17, 2019

Choose a reason for hiding this comment

ducky64 commented Jan 18, 2019

hngenc Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

hngenc commented Jan 18, 2019

ducky64 commented Jan 19, 2019

ducky64 commented Jan 19, 2019

chick left a comment

Choose a reason for hiding this comment

ducky64 commented Jan 22, 2019

hngenc Jan 18, 2019 •

edited

Loading