Thoughts on Treadle Options/Annotations Refactor

treadle-annos-feedback.md

This looks very good! I'm fine with it and the refactor aligns with what I've been pushing.

Some overall feedback along with some bigger thoughts follows.

As there are three locations in which information can exist, I'll use the following nomenclature:

• "case class option" -> a member of a case class used to represent options (e.g., topName of firrtl.FirrtlExecutionOptions)
• "CLI option" -> a command line argument
• "annotation" -> an annotation

Default Options (both case class and CLI)

A consistency problem arises when dealing with the default values for case class and CLI options. An example in treadle is TreadleExecutionOptions.rollBackBuffers/RollBackBuffersAnnotation. This has a default value, 4, but that is encoded in both the Annotation and in a default value for the case class. While there are several ways of solving this, I've used the convention of the Annotation, sometimes with the help of a private object that defines bogus mandatory arguments, will define it's default value and then the case class will use that to set a default value.

Consider the following structure:

• firrtl.FirrtlOptions.TargetDirAnnotation
• firrtl.FirrtlExecutionOptions

If you contrast that with TopNameAnnotation, that should not have a default value, but I've added a companion object with a private constructor that defines a bogus apply method. Note: that should probably just be a private object.

I've also been working with the following conventions:

• Boolean arguments default to true/false. The presence of an annotation changes this value. I currently have defaults that are respectively true and false. FIRRTL uses false options, Chisel uses true options. You seem to be doing this as well.
• All case class options that are non-Boolean are generally of type Option[T], unless they have a default value. I try to avoid, e.g., empty strings, so that you can differentiate value absence from some weird input value.

Library Registration

There's vestigial stuff from toy-options that prevents the library registration from working. The FIRRTL-backed library registration uses firrtl.options._ and not registration.spi._. If you swing that in META-INF, the registration should start working (and you'll actually be able to use CLI options!). Sorry that I didn't see that sooner...

This may change how you feel about usage of CLI options.

REPL looks okay!

The existing REPL helper treadle.sh just needs to point at treadle.TreadleReplDriver and it should be good to go. Some spot checking with one of the simple .fir files seems to work just fine. The FIRRTL refactor to pass annotations enables this to work. Previously, with the "view" happening inside an options manager, the options manager had to be correct at the time of construction. Now, an AnnotationSeq that would fail view[FirrtlExecutionOptions] can be passed around safely.

Tangentially, I would find a Chisel REPL immensely useful... I'm sure this has been discussed.

Redundancy of filename argument

I'm not a big fan of the (input: String, annotations: AnnotationSeq = Seq.empty) constructor because the input parameter is redundant on firrtl.InputFileAnnotation. Similarly, the inputs to ExecutionEngine's constructor seem like they could all be annotations. There may be some motivation to clean that up.

I'm biased, though. Everything at the tool level looks like an annotation to me. More discussion is needed on what is an annotation and what is not.

Redundant Annotations

Some of the annotations for various tools seem to be redundant with annotations that already exist or are just shims around other annotations. The ones that I've seen are the following, but there may be more:

• AllowCyclesAnnotation -> firrtl.DontCheckCombLoopsAnnotation
• VcdReplayFirrtlSourceNameAnnotation -> firrtl.InputFileAnnotation
• ReplFirrtlAnnotation -> firrtl.InputFileAnnotation

This may motivate a revival of commonOptions as CommonExecutionOptions?

View Checking

Currently, Treadle viewers are shims that functionally map annotations to case class options. It may be good to add in verification checks to make sure that valid options are passed.

I think all of these objections and solutions make sense, a couple of them I was already leaning towards and all of them I am in favor of (or of sitting and thinking of joining things under the common options in the case of combo loops etc.). I'll get to all of them next chance I have to sit and code for a couple of hours.