[major] Define option groups and instance choices

[nandor]

Add option and instance choice features to the FIRRTL spec. Define an ABI for these that uses files similar to how layers work.

There are more examples of syntax and possible ABIs in this repository: https://github.com/seldridge/instance-choice-abis

More Information

This feature exists to serve two separate purposes both related to allowing for limited parameterization of a FIRRTL circuit. This parameterization is later removed via specialization either: (1) by a FIRRTL compiler or (2) via Verilog elaboration.

Targets describe available paramterization as well as legal options for that target. (Names subject to change.) These target can then be used by FIRRTL constructs to describe behavior which is dependent on the value of a target.

Currently, only one construct can depend on the value of a target: instance choice. An instance choice is an instantiation of one of several modules where the choice of instantiation is controlled by the option selected for a given target. E.g., a likely target is one which captures "FPGA or ASIC or Generic" that, when set, can specialize the design for an FPGA, and ASIC, or give generic Verilog. E.g., a more "parameter-like" target would be one that sets the cache size.

All modules instantiated by an instance choice must have the exact same port-level interface including probes and properties. This is necessary to allow them to be substituted. The underlying hardware backing the probes and properties may vary from one instance choice to another. This is intentional and critical for this to be useful. Properties require no special treatment other than a specialization must be chosen before properties can be evaluated. Probes require tedious Verilog macros that allow the Verilog-level specialization to set the internal path.

[dtzSiFive]

Some nits/minor questions after a first pass.

[mmaloney-sf]

I am concerned about the orthogonality of this feature with external modules and intrinsic modules. It's hard for me to tell how this functionality is different in intent from those two features.

I also get a feeling this feature is overfitted to one particular usecase. Let's make sure we're being especially mindful of the complexity budget in FIRRTL. A feature like this feels to me like it might be better captured at the Chisel level. Are there arguments otherwise?

More detailed review is incoming.

[nandor]

I think we had a lengthy discussion about the need for this feature a while back, which I do not want to repeat here. External modules and intrinsic modules do not allow for the sort of optionality provided by this feature: we need to let users specialise for targets, FPGA and ASIC especially, without involving compiler-generated intrinsics or resolving the differences later via extmodules, out of the reach of the compiler.

It is not sufficient to capture this information at the Chisel generator level since then we can generate a design for only one of the options, we cannot encapsulate multiple possible targets as layers or this feature allow. At the Chisel level decisions can only be made if information is propagated through to the instance site, something that is not always possible, particularly in the case of the tool we are building this for. This warrants a feature which makes the decision based on a global flag somewhere late in the pipeline.

[seldridge]

This should have an ABI doc before merging.

I'd also like to get @darthscsi and @mmaloney-sf reviews.

[seldridge]

Thoughts on option and case @mmaloney-sf, @darthscsi?

[mmaloney-sf]

I don't like the use of the word option for this notion.

If I can choose between FPGA and ASIC, I would say "I have two options" not "I have two cases".

[seldridge]

Maybe target and option or variant:

circuit:
  target Platform:
    option FPGA
    option ASIC

This could also avoid the option entirely and instead use some array-esque syntax:

circuit:
  target Platform = [FPGA, ASIC]
  target Platform = {FPGA, ASIC}

There still needs to be a name for what "FPGA" is when describing it. Is it an "Platform option"? Is it a "Platform variant"? Is it a sub-target?

This notion of "targets" pushes towards Chisel likely defining default "targets" just like Chisel is expected to define default "layers". Users can always deviate from this. However, we can then make it part of the ABI where you can compile "target-independently" (delaying this to Verilog elaboration), "target-dependently", or "partially target-dependently" if only some of the targets are specified to the compiler.

I don't totally like this as it couples the ABI to a compilation option.

This could also go in the direction of this is a "circuit parameter".

[seldridge]

Why properties given that these are "evaluated" later?

Why no probes? It seems like this could be made to work if the define for a reference included another define that the optional instance must define?

[nandor]

They can be made to work, but it's not clear to me how at the moment and the feature is useful even without them.

[mmaloney-sf]

What does "not allowed" mean? We have to raise the bar in terms of precision with our spec. Phrases like this are not hard to interpret, but they introduce the risk that two people will interpret them differently. (Or just as bad, interpret them wrong).

Does "not allowed" mean something to do with the types that may appear on the ports of the module? If so, let's spell out the rules in the spec.

Additionally, I've been working very hard to eliminate this kind of "negative rule" from the spec. (Eg, things like "you can't connect Analog). This case is probably benign, but in general, these are a major source of ambiguity in the spec, and I want everyone to be very mindful of them.

[seldridge]

@rwy7: FYI, give this some thought about composition with true Verilog width parameterization.

[mmaloney-sf]

I know we have an internal usecase for this. But let's make sure we have the abstraction really crisp before pushing it forward. Especially if we could "fake it" with external modules and annotations in the meantime.

[mmaloney-sf]

What is the name of this feature? Option group? Or instance choice?

In either case, these terms are very generic. We should consider looking for a more textured name for this feature. (Similar to how @seldridge's "groups" were renamed to "layers").

[mmaloney-sf]

It would be good to add some copy distinguishing this feature from external modules, since both can be used to achieve the same effect.

If we struggle to write out this section adequately, it would be evidence that we haven't gotten to the correct abstraction.

Let's make sure we explain the distinction between ext modules and option groups to a general audience. If someone at Foobar, Inc. looking to use Chisel should be able to understand why we went to the effort to support both features.

[nandor]

I do not see the parallel to extmodules and this point keeps popping up! This is pretty much a sum type, a fundamentally different language concept. Please clarify why do you see this connection so we can settle this.

[mmaloney-sf]

Here, I would state the problem, then state the solution. Something like:

"It's very common to want to select between two implementations depending on the target platform (eg, FPGA vs ASIC). For example, you may want to use a block RAM inside of an FPGA, but for your ASIC design, you will use a module created by a memory compiler. For these situations, we can use $FEATURE_NAME to select between ..."

[mmaloney-sf]

but they can also be used specify parameters which are present in the output and can be selected after emission (for example, to tweak a design for size or performance).

This is a single sentence, but contains four or five different ideas. Please write each out separately for clarity.

Are we introducing a second usecase here? If we have two usecases, use two sentences. (Or perhaps even two paragraphs).

specify parameters

What notion of "parameters" are we referring to here? FIRRTL has no notion of parameters internally. If you're referring to Verilog parameters, you should say so.

can be selected after emission

What can be selected? The option group? The parameters?

after emission

What is "emission"? (As a note to others, esp @seldridge and @darthscsi, we might want to standardize on language in the ABI doc for this kind of thing).

(for example, to tweak a design for size or performance).

Again, split this out into its own sentence. Or two.

Also, what does "tweak a design" mean?

[mmaloney-sf]

I don't like the use of the word option for this notion.

If I can choose between FPGA and ASIC, I would say "I have two options" not "I have two cases".

[mmaloney-sf]

inst_choice clock_gate option Platform
  when FPGA FPGATarget default DefaultTarget

This syntax is completely unintuitive to me.

[mmaloney-sf]

Additionally, now that I've made sense of it, the syntax seems to imply that the choice of which instance to use can only depend on a single option. That seems silly to me.

If that's the case, it makes me wonder if what we want is a unified (and much simpler) notion of a "Platform".

Do we really need a default? What does the default mean? I don't see it commented on in the copy below.

Why is this done at the module instance level and not at the module declaration level? That seems more natural. At the very least we should motivate why this is the thing we want. (But my feeling is we shouldn't allow for this to vary instance by instance).

[nandor]

It is not 'silly' and please refrain from using such terms in written communication, as this was the result of somewhat careful consideration.

Each option defines an orthogonal dimension of configuration, implying the possibility of composition. We opted to make it explicit, limiting the operation to a choice along a single direction. This leaves composition to the user, who is now responsible for building the appropriate chain of instance choices if that is desired. Alternatively, we risk turning this into a full-blown pattern matching operation.

Since this is machine produced/consumed I do not see problems with the syntax, but I'm open to suggestions on how to improve it. My priority was to avoid introducing new keywords as much as possible.

[mmaloney-sf]

What does "not allowed" mean? We have to raise the bar in terms of precision with our spec. Phrases like this are not hard to interpret, but they introduce the risk that two people will interpret them differently. (Or just as bad, interpret them wrong).

Does "not allowed" mean something to do with the types that may appear on the ports of the module? If so, let's spell out the rules in the spec.

Additionally, I've been working very hard to eliminate this kind of "negative rule" from the spec. (Eg, things like "you can't connect Analog). This case is probably benign, but in general, these are a major source of ambiguity in the spec, and I want everyone to be very mindful of them.

[nandor]

I pushed a rewrite. I would want to focus on the syntax (as I am fairly open to changes) and I want to settle the debate about extmodules. Implementation-wise, an op in the FIRRTL dialect is necessary, but I can move forward with that.

I am approaching this problem from the perspective of algebraic data types/sum types, hence the terminology.

[mwachs5]

Suggested change

	Specialization is not mandatory: options can be left unspecified, resorting to explicitly-defined default behaviour.
	Specialization is not mandatory: options can be left unspecified, resorting to explicitly-defined default behavior.

nit: american spelling

[mwachs5]

Suggested change

	A default module is provided, to be instantiated when the design is not specialized or it is not specialised for a known case.
	A default module is provided, to be instantiated when the design is not specialized or it is not specialized for a known case.

[mwachs5]

is inference (Reset, width) allowed across these ports? If so must they resolve to the same thing for all choices?

[nandor]

Yes, I think these ports should not interfere with inference, as long as they resolve to the same width in all of the targeted modules.

[dtzSiFive]

Drop this sentence, maybe this paragraph?

[dtzSiFive]

Do targets themselves have a default value re:their options? (could they?)
I see that instance-choice have a "default" behavior when target is "unspecified" but that's not a value for a target and as written is explicitly none of the enumerated options, those are all for naming the non-default states.

So if these /were/ emitted as parametric Verilog, we'd need to have implicit default value made explicit.

A different design point is to require all values be enumerated, and probably the target itself indicates its default value. This fits with the framing of this as a sort of parameter --as "default" means "pick the default value" and isn't a value itself, at least in this framing :).

This way all states are explicit and there's vocabulary for the default state, possibly a name that isn't known only as "unspecified" (such that "default" Platform could be named, not just the absence of being FPGA or ASIC), and flows/code working with this can just request "Target=X".

This does have (hopefully minor?) implications re:macro / ABI details, but curious what you think about this and curious why it's done this way?

[seldridge]

This was inherited from the original proposal. However, it occurred to me that the default could be made explicit.

There's two ABI concerns, one current and one future, that motivate some notion of a default:

1. (Current) A user shouldn't need to do anything to "specialize" for the default. Ergo, the mechanism of enabling the default target should be "no action". With the file-based ABI this would naturally mean "no additional files are included in the build."
2. (Future) Verilog parameters must have a default value.

I do think it's entirely reasonable to roll the default into the target. Part of the changes here would be to decouple the target from instance choice and it therefore seems weird to handle the default (a target concern) with the instance choice.

I think this means either every target has a reserved "default" option or the target indicates its default:

target Platform:
  option Generic ; default is first
  option FPGA
  option ASIC

target Platform:
  option FPGA
  option ASIC
  ; There is an implicit option "default" that can be set for any Platform

target Platform = Generic:
  alternative FPGA
  alternative ASIC

WDYT syntax-wise?

For these, it would then be easiest to state that either instance choices must be fully specified or that the default is taken for any option not specified:

instchoice foo, Platform:
  FPGA => Bar
  ASIC => Foo
  default => Foo

instchoice foo, Platform:
  default => Foo
  FPGA => Bar

[dtzSiFive]

Are targets design-wide? Should they be? Or can they be set per-FIRRTL component, or maybe even per (public?) module (doesn't seem to be broken apart in that way, so I guess per-translation-unit)? If I specialize one component, can I use it with another component that has that option that hasn't been specialized?

As-is I think you could include a target_Target_A.sv for component X and target_Target_B.sv for component Y (different FIRRTL files) and since all macros are in terms of specific names within each, they wouldn't conflict. Maybe they should? (or at least let's be sure we're clear on whether this is intentional/supported or explicitly not).

On that, why are the macros per-module-instance (appears to be lowercase of at least the module name?), if they're bulk-enabled all at once?

Thinking out loud a smidge -- so if these just set a single macro (or otherwise manipulate the global environment pre-elaboration) like define Target A... the SV for each instance choice would then have effectively its dispatch table locally (vs sunk into/across the Target headers, for better or for worse).

Might be interesting to look at how neat we could make that with some light parameter/enum/generate emission, to perhaps avoid some messier ifdef chain. Offhand I think we'd have copies of the instantiation statements but I'm not sure that's a big loss and might be more readable when debugging / locally inspectable.

It also makes explicitly that this is closed and must be one of those options, and makes that locally visible.

Hmm.

[seldridge]

They're design-wide/bulk-enabled just like layers. They can be set per-component, but only if a FIRRTL circuit is organized to make that possible. E.g., if you want to specialize every instance of a module, you are really describing different modules with different instance choices set by different targets. This is also motivated by the fact that I do not know of a way in Verilog to achieve per-instance specialization. There may be ways to do this that are tool-specific.

It probably needs to be per-public module. However, there are some problems here for a module which has an instance choice which is instantiated by two public modules where one public module instantiates the other. In this situation, you need to give the option of enabling the instance choice from each public module (via two files). However, the two public modules have to agree on the choice.

This could be avoided if public modules were not allowed to instantiated any common submodules. It could also be avoided with generate and parameters at the cost of exposing all instance choices in the design and not allowing easy directory pruning specialization.

[seldridge]

I updated this to put the module name in the file.

The issue of multiple public modules needing to agree is actually a red herring. While having multiple public modules is possible, a Verilog elaboration is always a tree rooted at a FIRRTL public module. Therefore, I think the only checking that needs to be done is if a macro is multiply defined.

E.g., an error is possible for a design like the following with public modules Foo, Bar, and Baz. Both Bar and Baz instantiate Qux which has an instance choice of Qux or Corge.

The problem here is that there are six specialization files (Cartesian product [Foo, Bar, Baz] x [Quz, Corge]). Some of these are compatible and some are incompatible. However, it is easiest to just make all of them incompatible in the ABI as it is likely wrong for such a circuit to ever be including specialization files for any module which is not the root, unless the user has guaranteed that they are using isolated instance graph trees. I.e., if the user wants to specialize Qux differently under the Bar and Baz modules, then Qux needs to be duplicated in the instance graph.

This is going to need to be handled carefully in the implementation of the ABI based on how the internal names of defines are chosen.

[dtzSiFive]

Maybe it's bikeshedding too much, but could you explain the thinking regarding naming the keyword and word used to describe one dimension of optionality as a "target"?

Target seems like a great and compelling example of the optionality we want to capture, and we might even want to capture different dimensions of targets (maybe), but I kinda like calling these "options" better. "case" works for me too, although on the subject -- I'm not sure they need keywords really vs just listing them like in an enum or something.

"Target" seems to imply a more narrow use/purpose and does not itself imply any optionality, and just to be frank but not argumentative, I found it a bit confusing at first/as a result.

Commonly you specify a target as a known thing your compiler understands and MAYBE it'll produce different code as a result but also maybe not....

Anyway, I don't understand this change, would you mind sparing some words to explain?

[seldridge]

The motivation to call this "target" and not "option" is primarily by the use case. These are intended to allow for specializing the design for some target purpose, feature, technology, etc. FPGA vs. ASIC vs. Generic is closer to the more specific "target" than it is to a generic "option". You can counter argue that "target" doesn't make sense for parametric features that this would enable like cache size.

This is also try to permute the discussion with alternative language in response to confusion like this: #155 (comment)

I agree that "case" or "option" is unnecessary as these are really just describing a sum type. Any of the following would be fine and don't add a second superfluous keyword:

option Target = FPGA, ASIC, Generic
option Target:
  FPGA
  ASIC
  Generic

WDYT?

[seldridge]

After some discussion with @rwy7, option seems like a better name to describe what is going on. I'll revise this and get this moving.