Add CustomRadixGTKWaveAnnotation to generate Tcl script for GTKWave

[sequencer]

depends on #2434 GTKWave will invoke script to generate readable signal below:

Contributor Checklist

• [x] Did you add Scaladoc to every public function/method?
• [ ] Did you update the FIRRTL spec to include every new feature/behavior?
• [x] Did you add at least one test demonstrating the PR?
• [x] Did you delete any extraneous printlns/debugging code?
• [x] Did you specify the type of improvement?
• [x] Did you state the API impact?
• [x] Did you specify the code generation impact?
• [x] Did you request a desired merge strategy?
• [x] Did you add text to be included in the Release Notes for this change?

Type of Improvement

• new feature/API

API Impact

Add API CustomRadix{Def,Apply}Annotation CustomRadixGTKWaveAnnotation and CustomRadixConfigFileAnnotation, Add command line API: --wave-viewer-script

Backend Code Generation Impact

None

Desired Merge Strategy

• Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

None

Reviewer Checklist (only modified by reviewer)

• [ ] Did you add the appropriate labels?
• [ ] Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
• [ ] Did you review?
• [ ] Did you check whether all relevant Contributor checkboxes have been checked?
• [ ] Did you mark as Please Merge?

[ekiwi]

Since ChiselEnum is a Chisel feature and not a firrtl feature, maybe it would be better to add the GTKWave support in the Chisel repository? What would be the advantage of moving all of this to the firrtl repo?

[ekiwi]

If you are working on better GTKwave integration it might be worth having a look at what the BlueSpec folks have been working on: https://github.com/B-Lang-org/bdw/blob/75bb68c5e02c5fbab14c1fe0a8a1a5d95c1f8015/src/util/GtkWave.tcl#L309

[sequencer]

Since ChiselEnum is a Chisel feature and not a firrtl feature,

I think the essence of this annotation is providing a map from bit vector to string.

maybe it would be better to add the GTKWave support in the Chisel repository?

I think this should be a firrtl feature instead? In fact, ChiselEnum generate those mapping(annotation). How to use those mapping is firrtl’s job, so think it should exist in firrtl.

What would be the advantage of moving all of this to the firrtl repo?

gtkwave doesn’t depend on chisel. Any valid Annotation with correspond firrtl circuit can use this feature.

[sequencer]

If you are working on better GTKwave integration it might be worth having a look at what the BlueSpec folks have been working on: https://github.com/B-Lang-org/bdw/blob/75bb68c5e02c5fbab14c1fe0a8a1a5d95c1f8015/src/util/GtkWave.tcl#L309

Thank you! That will save a lot of time reading the gtkwave documentation!

[ekiwi]

I still think this should be part of Chisel and not firrtl. Generally I would prefer if chisel users never need to import things from the firrtl package. The firrtl backend should remain an implementation detail since eventually one might be able to use Chisel with CIRCT and no dependency on firrtl. So everything that users are expected to use directly should be in a chisel3 (sub)package.

[sequencer]

I still think this should be part of Chisel and not firrtl. Generally I would prefer if chisel users never need to import things from the firrtl package.

Chisel StrongEnum will depend on CustomRadix Annotations, and CustomRadix doesn't provide any chisel-related API. So I think this is reasonable to provide this as a standalone API in FIRRTL.

The firrtl backend should remain an implementation detail since eventually one might be able to use Chisel with CIRCT and no dependency on firrtl.

I think in the long-term decision, as @seldridge said, FIRRTL should improve the IR support to custom radix(enum), and directly emit correspond IR to FIRRTL/CIRCT.

So everything that users are expected to use directly should be in a chisel3 (sub)package.

Yes, user won't touch these APIs, they only use StrongEnum, and StrongEnum will generate correspond API, if user doesn't provide a CustomRadixConfigFileAnnotation, the CustomRadixTransform won't execute and there will be no behavior changes.

In general, I think in this PR, we only leverage the dependency from Chisel to FIRRTL, original StrongEnumAnnotation doesn't do any thing, so I think it's reasonable to re-implement this.

[sequencer]

It might be nice to generate a data file instead of the GTK .tcl and then instead provide a static .tcl file that reads the data file. This might extend better to other tools since someone using something proprietary could write their own .tcl to read the same data file.

We have tried, tcl sucks :( So we think a trade-off might be good: providing a JSON output by default, if other users prefer directly using the raw data. If user explicitly selected the gtkwave via args, the correspond tcl script will be emitted.

Another option is to look at generate .gtkw files directly. You can see what these are like by manually creating the mapping inside of an instance of GTKWave and then saving it to see what it looks like for the mappings you've created.

I investigated this, and found this might not be a good solution: one GTKWave session will only have one gtkw file, which makes it impossible to let user using their own gtkw(if they have), so executing a tcl might be a reasonable solution instead.

[jackkoenig]

I'm fine with this being in firrtl but I agree with @ekiwi that the user API needs to be fully in chisel3. The issue is the annotation to specify what file you want emitted. I think we can have a different annotation in chisel3 that turns into one of these firrtl annotations

[sequencer]

In my point of view, The reason I think this API should exist in FIRRTL is:

1. These codes doesn't depend on any Chisel codes.
2. User API is exposed via command-line args: (--wave-viewer-script, gtkwave), which can be directly used by any Chisel users. we can add docs to docs
3. If user choose to use annotation to generate the gtkwave tcl: they need to append these annotations:

Seq(
  firrtl.transforms.RunFirrtlTransformAnnotation(Dependency(CustomRadixTransform)),
  firrtl.transforms.GTKWaveCustomRadixScriptAnnotation(Seq.empty, Seq.empty)
)

AFAIK, This pattern is universally used in Chisel designs, for example: if user needs to select output dir: firrtl.stage.TargetDirAnnotation is required. if user needs to infer read-write of SRAM: firrtl.passes.memlib.InferReadWriteAnnotation, firrtl.passes.memlib.RunFirrtlTransformAnnotation(new InferReadWrite) are required if user needs to change log level: logger.LogLevelAnnotation(which is vended by firrtl) Those APIs are APIs in my daily uses, I usually import them from firrtl package. So I design this API via this guideline: common user can always access APIs from command-line without importing anything from firrtl. If user choose to explicit use the Annotation in their API, it's OK to add API which belongs to firrtl.

[jackkoenig]

@sequencer does this need to be rebased since https://github.com/chipsalliance/firrtl/pull/2434 was merged?

[sequencer]

Yes , it will be rebased to master. I thought this won’t block 3.5-RC2? It it did I’ll work on it today.

On Fri, Dec 17, 2021 at 16:09 Jack Koenig @.***> wrote:

@sequencer https://github.com/sequencer does this need to be rebased since #2434 https://github.com/chipsalliance/firrtl/pull/2434 was merged?

— Reply to this email directly, view it on GitHub https://github.com/chipsalliance/firrtl/pull/2397#issuecomment-996517765, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMF2K5GMXOAW7H3QMXGIO3URLV3JANCNFSM5G23GMGA . You are receiving this because you were mentioned.Message ID: @.***>

[sequencer]

I’m still finding a better way to invoke gtkwave ;p

On Fri, Dec 17, 2021 at 16:16 Jiuyang Liu @.***> wrote:

Yes , it will be rebased to master. I thought this won’t block 3.5-RC2? It it did I’ll work on it today.

On Fri, Dec 17, 2021 at 16:09 Jack Koenig @.***> wrote:

@sequencer https://github.com/sequencer does this need to be rebased since #2434 https://github.com/chipsalliance/firrtl/pull/2434 was merged?

— Reply to this email directly, view it on GitHub https://github.com/chipsalliance/firrtl/pull/2397#issuecomment-996517765, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMF2K5GMXOAW7H3QMXGIO3URLV3JANCNFSM5G23GMGA . You are receiving this because you were mentioned.Message ID: @.***>

[jackkoenig]

I thought this won’t block 3.5-RC2

Don't worry, it's not blocking RC2! I've just been looking through PRs so thought I'd follow up!

[sequencer]

Ok! I’ll finish it this weekend ;p

On Fri, Dec 17, 2021 at 16:23 Jack Koenig @.***> wrote:

I thought this won’t block 3.5-RC2

Don't worry, it's not blocking RC2! I've just been looking through PRs so thought I'd follow up!

— Reply to this email directly, view it on GitHub https://github.com/chipsalliance/firrtl/pull/2397#issuecomment-996525956, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMF2KZQZWEADY36VUXTVUTURLXRZANCNFSM5G23GMGA . You are receiving this because you were mentioned.Message ID: @.***>