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

Should the File Preamble be enhanced to include the ABI version? #132

Open
smarter opened this issue Oct 18, 2023 · 5 comments
Open

Should the File Preamble be enhanced to include the ABI version? #132

smarter opened this issue Oct 18, 2023 · 5 comments

Comments

@smarter
Copy link
Contributor

smarter commented Oct 18, 2023

https://github.com/chipsalliance/firrtl-spec/blob/main/abi.md defines both ABIv1 and ABIv2, however there does not seem to be a way to specify which version is expected for a given .fir file. It seems to me that this information should be part of the File Preamble, e.g.:

FIRRTL version 3.2.0-abiv2

Otherwise there is no way to know how to actually interop with a .fir file without external information.

@seldridge
Copy link
Member

This is an insufficiently specified area. 😅 Yes, the ABI version needs to also be specified. The ABI also needs to be specified on external modules as that represents something which can be implemented by another circuit:

FIRRTL version 3.2.0-abiv1
circuit Foo:
  extmodule Bar:
    abi = v2
    output a: UInt<1>[1]
    
  module Foo:
    output a: UInt<1>[1]

    inst bar of Bar
    connect a, bar.a

For FIRRTL spec version 4.0+ (which will introduce public modules: #130), there is a question of if the ABI should be specified per-public module or per-circuit. This is the only reason why I would hold back on putting this in the FIRRTL version preamble if we wanted to go with the per-module approach:

FIRRTL version 4.0.0
circuit Foo:
  public module Bar:
    abi = v2
    input a: UInt<1>[1]

  public module Baz:
    abi = v2
    input a: UInt<1>[1]

The per module approach aligns with existing language on "module conventions" (See: https://github.com/chipsalliance/firrtl-spec/blob/main/spec.md#module-conventions). However, this was written before the ABI v1/v2 part and is largely duplicative of what the ABI lays out more exactly.

WDYT?

@smarter
Copy link
Contributor Author

smarter commented Oct 18, 2023

Ah, extmodule having to possibly specify a different ABI makes a lot of sense, I guess if we wanted to avoid repetitions, we could specify it at the circuit level and override it at the module level:

FIRRTL version 4.0.0
circuit Foo:
  abi = v2

  extmodule Bar:
    abi = v1
    output a: UInt<1>[1]
    
  public module Foo:
    output a: UInt<1>[1]

    inst bar of Bar
    connect a, bar.a

But having to specify it for every public module seems fine too.

@smarter
Copy link
Contributor Author

smarter commented Oct 18, 2023

Not sure where to ask this, but is ABIv2 expected to match the behavior of firtool --preserve-aggregate=all --scalarize-top-module=false ?

@seldridge
Copy link
Member

seldridge commented Oct 18, 2023

Not sure where to ask this, but is ABIv2 expected to match the behavior of firtool --preserve-aggregate=all --scalarize-top-module=false ?

Yes. Though, this is a very untested path this path is not load bearing for anyone that I know of.

@darthscsi
Copy link
Collaborator

It's reasonable to specify ABI for any module with a circuit-level default. I had imagined it being a compile time option, but it makes sense to be per-module.

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

No branches or pull requests

3 participants