-
Notifications
You must be signed in to change notification settings - Fork 177
Multi protobuf module emission and consumption #2344
Multi protobuf module emission and consumption #2344
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good although a few minor issues. Also obviously needs tests but it is a Draft for now 🙂
Co-authored-by: Jack Koenig <[email protected]>
new ShellOption[String]( | ||
longOption = "input-directory", | ||
toAnnotationSeq = a => Seq(FirrtlDirectoryAnnotation(a)), | ||
helpText = "A directory of FIRRTL files", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these be either .fir
or .pb
files? A mixture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should figure that out. I think to start we should make it be only .fir
or only .pb
files. Note that -i
actually doesn't care about file extensions, it just tries to deserialize as protobuf and if it fails it deserializes as .fir
but I think it's okay to require correct file extensions here.
Co-authored-by: Jack Koenig <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and a nice suite of tests! Just some suggestions for improved error messages then let's get this merged.
Co-authored-by: Jack Koenig <[email protected]>
* Add compiler option (`-p`) to emit individual module protobufs * Implement multi module combination when reading directory of protobufs Co-authored-by: Jack Koenig <[email protected]> (cherry picked from commit 0c1ca58)
* Multi protobuf module emission and consumption (#2344) * Add compiler option (`-p`) to emit individual module protobufs * Implement multi module combination when reading directory of protobufs Co-authored-by: Jack Koenig <[email protected]> (cherry picked from commit 0c1ca58) * Multi PB backport fixes (#2353) Co-authored-by: Jared Barocsi <[email protected]>
Contributor Checklist
Type of Improvement
API Impact
No API impact
Backend Code Generation Impact
Can change ordering of modules.
MultiInfo
s do not serialize properly because of placeholder code in the protobuf emitter.Desired Merge Strategy
Release Notes
Implement multiple protobuf emission and consumption in the FIRRTL compiler
-p
option exports all modules into individual protobuf files, similar to-e
.-I
option takes a directory of FIRRTL files, deserializes them and combines them back into a single circuit. Currently supports only the protobuf files emitted by-p
Reviewer Checklist (only modified by reviewer)
Please Merge
?