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

[major] Define option groups and instance choices #155

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions abi.md
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,136 @@ bind Bar Bar_Layer1_Layer2 layer1_layer2(.notA(Bar.layer1.notA));
The `` `ifdef ``{.verilog} guards enable any combination of the bind files to be included while still producing legal SystemVerilog.
I.e., the end user may safely include none, either, or both of the bindings files.

## On Targets

A target will result in the creation of specialization files.
One specialization file per public module per option for a given target will be created.

Including a specialization file in the elaboration of Verilog produced by a FIRRTL compiler specializes the instantiation hierarchy under the associated public module with that option.

Each specialization file must have the following filename where `module` is the name of the public module, `target` is the name of the target, and `option` is the name of the option:

``` ebnf
filename = "targets_" , module , "_" , target , "_", option , ".vh" ;
```

Each specialization file will guard for multiple inclusion.
Namely, if two files that specialize the same target are included, this must produce an error.
If the file is included after a module which relies on the specialization, this must produce an error.

### Example

The following circuit is an example implementation of how a target can be lowered to align with the ABI defined above.
This circuit has one target named `Target` with two options, `A` and `B`.
Module `Foo` may be specialized using an instance choice that will instantiate `Bar` by default and `Baz` if `Target` is set to `A`.
If `Target` is set to `B`, then the default instantiation, `Bar`, will occur.
Module `Foo` includes a probe whose define is inside the instance choice.

``` firrtl
FIRRTL version 4.0.0
circuit Foo:

target Target:
option A
option B

module Baz:
output a: Probe<UInt<1>>

node c = UInt<1>(1)
define a = probe(c)

module Bar:
output a: Probe<UInt<1>>

node b = UInt<1>(0)
define a = probe(b)

public module Foo:
output a: Probe<UInt<1>>

instchoice x of Bar, Target:
A => Baz

define a = x.a
```

To align with the ABI, this must produce the following files to specialize the circuit for option `A` or option `B`, respectively:

- `targets_Foo_Target_A.vh`
- `targets_Foo_Target_B.vh`

What follows describes a possible implementation that aligns with the ABI.
Note that the internal details are not part of the ABI.

When compiled, this produces the following Verilog:

``` systemverilog
module Baz(output a);
assign a = 1'h1;
endmodule

module Bar(output a);
assign a = 1'h0;
endmodule

// Defines for the instance choices
`ifndef __target_Target_foo_x
`define __target_Target_foo_x Bar
`endif

module Foo();

`__target_Target_foo_x x();

endmodule
```

The contents of the two option enabling files are shown below:

``` systemverilog
// Contents of "targets_Target_A.vh"
`ifdef __target_Target_foo_x
`ERROR__target_Target_foo_x__must__not__be__set
`else
`define __target_Target_foo_x Baz
`endif

`ifdef __targetref_Foo_x_a a
`ERROR__targetref_Foo_x_a__must__not__be__set
`else
`define __targetref_Foo_x_a a
`endif
```

``` systemverilog
// Contents of "targets_Target_B.vh"
`ifdef __target_Target_foo_x
`ERROR__target_Target_foo_x__must__not__be__set
`endif

// This file has no defines.
```

Additionally, probe on public module `Foo` requires that the following file is produced:

``` systemverilog
// Contents of "refs_Foo.sv"
`ifndef __targetref_Foo_x_a
`define __targetref_Foo_x_a b
`endif

`define ref_Foo_x x.`__targetref_Foo_x_a
```

If neither of the option enabling files are included, then `Bar` will by default be instantiated.
If `targets_Foo_Target_A.vh` is included before elaboration of `Foo`, then `Baz` will be instantiated.
If `targets_Foo_Target_B.vh` is included before elaboration of `Foo`, then `Bar` will be instantiated.
If both `targets_Foo_Target_A.vh` and `targets_Foo_Target_B.vh` are included, then an error (by means of an undefined macro error) will be produced.
If either `targets_Foo_Target_A.vh` or `targets_Foo_Target_B.vh` are included after `Foo` is elaborated, then an error will be produced.

If `ref_Foo.vh` is included before either `targets_Foo_Target_A.vh` or `targets_Foo_Target_B.vh`, then an error will be produced.

## On Types

Types are only guaranteed to follow this lowering when the Verilog type is on an element which is part of the ABI defined public elements.
Expand Down
3 changes: 3 additions & 0 deletions include/firrtl.xml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
<item>extmodule</item>
<item>intmodule</item>
<item>layer</item>
<item>target</item>
</list>
<list name="keywords">
<item>input</item>
Expand All @@ -27,6 +28,8 @@
<item>propassign</item>
<item>public</item>
<item>enablelayer</item>
<item>instchoice</item>
<item>option</item>
</list>
<list name="types">
<item>UInt</item>
Expand Down
2 changes: 2 additions & 0 deletions revision-history.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@ revisionHistory:
- Main module must be public.
- Make commas mandatory, not whitespace.
- Update intrinsic module example to use a real intrinsic.
- Add instance choice.
abi:
- Add ABI for public modules and filelist output.
- Changed ABI for group and ref generated files.
These now use the public module and not the circuit.
- Use EBNF to describe probe port macros and filename.
- Correct mistakes in code examples.
- Add instance choice.
# Information about the old versions. This should be static.
oldVersions:
- version: 3.2.0
Expand Down
87 changes: 87 additions & 0 deletions spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,40 @@ circuit Foo :
public module Foo enablelayer A :
```

## Targets
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?


A `target`{.firrtl} describes one way that a FIRRTL circuit may be specialized for a certain use case.
Here, specialization means choosing a specific option for a target.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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


It is often desirable to have one FIRRTL circuit that has different logic when simulated, synthesized and mapped to a field-programmable gate array (FPGA), or synthesized to a given process technology and standard cell library.
While this per-target customizability can be expressed and specialized in a frontend language that produces FIRRTL, it is often desirable to expose the target specialization in the FIRRTL.
By delaying the specialization, the specialization can either be done by a FIRRTL compiler or exposed in the artifacts of a FIRRTL compiler for specialization by the consumer.

Practically, targets describe a limited form of parameterization and, if not specialized by a FIRRTL compiler, allow for FIRRTL compilers to generate parametric artifacts (e.g., parametric Verilog).

The `option`{.firrtl} keyword declares an option group, which contains `case`{.firrtl} declarations naming the settings allotted to that option.
Copy link
Member

Choose a reason for hiding this comment

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

Drop this sentence, maybe this paragraph?

The circuit can be specialized for a single case of a given option at any time.
Multiple option groups can be declared to capture orthogonal dimensions of configuration.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

image

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.


Specialization can occur either in the compiler or it can be materialized in the lowering.
For details, consult the FIRRTL ABI specification.
Specialization is not mandatory: options can be left unspecified, resorting to explicitly-defined default behaviour.

A target may be declared using the `target`{.firrtl} keyword.
The available options for which a target may take are listed using the `option`{.firrtl} keyword.
An example FIRRTL circuit showing two targets, `Platform` and `Performance`, and their allowable options is shown below:

``` firrtl
circuit:
target Platform:
option FPGA
option ASIC

target Performance:
option Slow
option Fast
```

# Circuit Components

Circuit components are the named parts of a module corresponding to hardware.
Expand Down Expand Up @@ -481,6 +515,47 @@ circuit Foo:
;; snippetend
```

#### Instance Choice

An instance choice is a submodule instance where the choice of submodule is conditioned based on the value of a `target`{.firrtl}.
This enables per-instance specialization for different targets.
Additionally, this is a mechanism for module replacement.

An instance choice declaration specifies the instance name and names the option group based on which the choices are selected.
A default module must be provided.
The default module is instantiated by the instance choice when an option for its associated target is not specified.
Subsequently, modules can be specified for the known choices of the selected option group.
An instance choice does not need to specify modules for all cases.
The instantiated modules must be either modules or external modules.

An example of an instance choice is shown below.
This instance choice is conditioned on the `Platform` target.
By default, it will instantiate `DefaultClockGate` and when `Platform` is `FPGA` it will instantiate `FPGAClockGate`.

``` firrtl
circuit:
target Platform:
option FPGA
option ASIC

module DefaultClockGate:
input clock_in: Clock
output clock_out: Clock
input enable: UInt<1>

extmodule FPGAClockGate:
input clock_in: Clock
output clock_out: Clock
input enable: UInt<1>

module InstanceChoice:
instchoice clock_gate of DefaultClockGate, Platform:
FPGA => FPGAClockGate
```

The type of an instance choice is the same as an instantiation of the default module.
The ports of all module choices must be the same as the default module.

nandor marked this conversation as resolved.
Show resolved Hide resolved
### Memories

Memories are stateful elements of a design.
Expand Down Expand Up @@ -4128,6 +4203,7 @@ decl =
| decl_extmodule
| decl_intmodule
| decl_layer
| decl_target
| decl_type_alias ;

decl_module =
Expand Down Expand Up @@ -4156,6 +4232,11 @@ decl_layer =
{ decl_layer , newline } ,
dedent ;

decl_target =
"target" , id , ":" , [info] , newline, indent ,
{ "option" , id , ":" , [ info ] , newline } ,
dedent ;

decl_type_alias = "type", id, "=", type ;

port = ( "input" | "output" ) , id , ":" , (type | type_property) , [ info ] ;
Expand All @@ -4177,11 +4258,17 @@ circuit_component =
| circuit_component_wire
| circuit_component_reg
| circuit_component_inst
| circuit_component_instchoice
| circuit_component_mem ;

circuit_component_node = "node" , id , "=" , expr , [ info ] ;
circuit_component_wire = "wire" , id , ":" , type , [ info ] ;
circuit_component_inst = "inst" , id , "of" , id , [ info ] ;
circuit_component_instchoice =
"instchoice" , id , "of" , id , "," , id , ":" , newline ,
indent ,
{ id , "=>" , id , newline } ,
dedent;

circuit_component_reg =
"reg" , id , ":" , type , expr , [ info ]
Expand Down