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] Move Extmodule Params to Instantiation #46

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

seldridge
Copy link
Member

This changes the format for FIRRTL external modules to move parameter values to the site of instantiation as opposed to being declared along with the extmodule definition. This has the added benefit of removing the "defname" as this is now unified with the name of the external module.

Signed-off-by: Schuyler Eldridge [email protected]

This changes the format for FIRRTL external modules to move parameter
values to the site of instantiation as opposed to being declared along
with the extmodule definition.  This has the added benefit of removing
the "defname" as this is now unified with the name of the external
module.

Signed-off-by: Schuyler Eldridge <[email protected]>
@seldridge
Copy link
Member Author

This is only a sliver of where I and others (@darthscsi 👀 ) want to take parameters. However, this is the bare minimum cleanup that fixes a lot of weirdness around external modules. This also matches where I want to take the internal MFC representation of external modules.

Copy link
Collaborator

@mwachs5 mwachs5 left a comment

Choose a reason for hiding this comment

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

It seems like we need to support / demonstrate parameters in UInt widths at least to make this change?

```

The widths of all externally defined module ports must be specified. Width
inference, described in [@sec:width-inference], is not supported for module
ports.

Externally defined modules may have zero or more parameters. Parameters may be
of known-width `UInt`{.firrtl} or `SInt`{.firrtl} types or `String`{.firrtl}
type. The value of a parameter is set at each instantiation of an external
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we mention that they must be passed in declaration order (there is no name-specified parameter passing syntax)

spec.md Show resolved Hide resolved
@darthscsi
Copy link
Collaborator

It seems like we need to support / demonstrate parameters in UInt widths at least to make this change?

Yes, the extmodules are currently expressing the result of their parameterization, which needs to be fixed before we can move the parameters to the instances.

@seldridge
Copy link
Member Author

It seems like we need to support / demonstrate parameters in UInt widths at least to make this change?

Yes. I totally missed this. Currently you can do this with multiple extmodules with the same defname, different parameters, and different widths. This is going to bloat this PR a bit as it gets into parameter expression operations. More to follow...

The most basic example of this is we need to deal with something like:

extmodule Bar1:
  input a: UInt<4>
  defname Bar
  param width = 4
extmodule Bar2:
  input a: UInt<8>
  defname Bar
  param width = 8

This needs to be represented as something like:

extmodule Bar<parameter width: UInt<32>>:
  input a: UInt<width>

Make parameter types explicitly boxed.  Restrict the locations where
parameter types can show up to nodes and external module parameters.
Allow aggregate, passive parameter types.

Add a new parameter primitive operation section.  This is presently
incomplete, but includes a single `primAdd` operation to show how this
would work.

Update the EBNF grammar and the FIRRTL syntax highlighting XML file.
@seldridge
Copy link
Member Author

I updated this with a couple of changes:

  1. Parameters are now boxed with a Param<...> indicator. This is done to sequester parameters into their own area.
  2. To solve the issue highlighted in the above comment ([major] Move Extmodule Params to Instantiation #46 (comment)) as brought up by Megan and Lenharth, I've added an incomplete section on parameter primitive operations. This only shows paramAdd and would need to include more operations to be actually usable. FIRRTL does not normally create new ops for new types. It would be more consistent with the existing FIRRTL spec to define these on existing primitive operations.

There issues that need to be ironed out related to parameter directionality. E.g., are there input and output parameters?

As it came up in offline discussions, this change has impacts on how deduplication works. However, I don't expect it to change any deduplication behavior. Examples follow:

Example 1

In the following circuit, Bar and Baz will not deduplicate:

circuit Top1:
  extmodule Foo<parameter x: UInt<8>> :
    output a: UInt<1>

  module Bar:
    output a: UInt<1>

    inst foo of Foo<4>
    a <= foo.a

  module Baz:
    output a: UInt<1>

    inst foo of Foo<8>
    a <= foo.a

  module Top1:
    output a: UInt<1>

    inst bar of Bar
    inst baz of Baz
    a <= xor(bar.a, baz.a)

This is equivalent to the current situation now. The difference is that inst foo of Foo_4/inst foo of Foo_8 are replaced with inst foo of Foo<4>/inst foo of Foo<8>:

circuit Top1:
  extmodule Foo_4:
    output a: UInt<1>
    defname = Foo
    parameter x = 4

  extmodule Foo_8:
    output a: UInt<1>
    defname = Foo
    parameter x = 8

  module Bar:
    output a: UInt<1>

    inst foo of Foo_4
    a <= foo.a

  module Baz:
    output a: UInt<1>

    inst foo of Foo_8
    a <= foo.a

  module Top1:
    output a: UInt<1>

    inst bar of Bar
    inst baz of Baz
    a <= xor(bar.a, baz.a)

Example 2

If, on the other hand, the parameters passed to the external module instance are the same, then Bar and Baz will deduplicate:

circuit Top2:
  extmodule Foo<parameter x: UInt<8>> :
    output a: UInt<1>

  module Bar:
    output a: UInt<1>

    inst foo of Foo<4>
    a <= foo.a

  module Baz:
    output a: UInt<1>

    inst foo of Foo<4>
    a <= foo.a

  module Top2:
    output a: UInt<1>

    inst bar of Bar
    inst baz of Baz
    a <= xor(bar.a, baz.a)

This is equivalent to the current situation where an external module is instantiated twice:

circuit Top2:
  extmodule Foo_4:
    output a: UInt<1>
    defname = Foo
    parameter x = 4

  module Bar:
    output a: UInt<1>

    inst foo of Foo_4
    a <= foo.a

  module Baz:
    output a: UInt<1>

    inst foo of Foo_4
    a <= foo.a

  module Top2:
    output a: UInt<1>

    inst bar of Bar
    inst baz of Baz
    a <= xor(bar.a, baz.a)

Or where the external module is duplicated and should deduplicate:

circuit Top2:
  extmodule Foo_4_0:
    output a: UInt<1>
    defname = Foo
    parameter x = 4

  extmodule Foo_4_1:
    output a: UInt<1>
    defname = Foo
    parameter x = 4

  module Bar:
    output a: UInt<1>

    inst foo of Foo_4_0
    a <= foo.a

  module Baz:
    output a: UInt<1>

    inst foo of Foo_4_1
    a <= foo.a

  module Top2:
    output a: UInt<1>

    inst bar of Bar
    inst baz of Baz
    a <= xor(bar.a, baz.a)

Example 3

There will need to be canonicalization/folding of parameters in order to cause situations like the following to properly recognize that two instances are the same. In the worst case, this requires walking backwards across expressions until you hit a root Param literal (or a root Param in a world where modules are also parameterized). In practice, this is handled automatically with canonicalization.

circuit Top3:
  extmodule Foo<parameter x: UInt<8>> :
    input a: UInt<1>

  module Bar:

    node a = Param(UInt<7>(2))
    node b = Param(UInt<7>(2))
    inst foo of Foo<paramAdd(a, b)>

  module Baz:

    node a = Param(UInt<7>(1))
    node b = Param(UInt<7>(3))
    inst foo of Foo<paramAdd(a, b)>

  module Top3:

    inst bar of Bar
    inst baz of Baz

a `Param`{.firrtl}` type. This indicates that this is a type that is used to
express parameterization. Parameter types may be used to express
parameterization on externally defined modules. Parameter types may be used as
nodes. Parameter types may be used in passive aggregates. All other uses of
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you show an example of "Parameter types may be used in passive aggregates"? what does that mean?

@@ -2910,6 +2976,17 @@ reference = id
| reference , "[" , int , "]"
| reference , "[" , expr , "]" ;

(* Parameter primitive operations *)
primop_param_2expr_keyword = "add" ;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
primop_param_2expr_keyword = "add" ;
primop_param_2expr_keyword = "paramAdd" ;

if I am reading this right?

## Parameter Types

A `UInt`{.firrtl}, `SInt`{.firrtl}, or `String`{.firrtl} type may be boxed with
a `Param`{.firrtl}` type. This indicates that this is a type that is used to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
a `Param`{.firrtl}` type. This indicates that this is a type that is used to
a `Param`{.firrtl} type. This indicates that this is a type that is used to

@@ -27,6 +27,7 @@
<item>Clock</item>
<item>Reset</item>
<item>AsyncReset</item>
<item>Param</item>
Copy link
Member

@dtzSiFive dtzSiFive Jan 26, 2023

Choose a reason for hiding this comment

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

Nit/FWIW: Param is special as it wraps other types. I ran into this for Ref and wanted the inner types formatted so handled it like this: dtzSiFive/firrtl-spec@dtzSiFive:0190aba...dtzSiFive:abdbca8#diff-fc53c821ad3e7a3caa0a87f0f46f48202fca3bc55dbb9e56fc34cb0c6ec3ead3 . I think same would work for Param, as another "outertype", and while I think that could be narrowed a bit regardless we can probably use same approach here.

(Not sure about the other grammar changes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants