-
Notifications
You must be signed in to change notification settings - Fork 177
Add Bundle Literals. #929
base: master-deprecated
Are you sure you want to change the base?
Add Bundle Literals. #929
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.
This looks pretty good to me but I will defer to @azidar and @jackkoenig
The Firrtl Spec should be changed with this PR I think.
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.
Overall looks fine as a first pass. A few high level requests/comments/questions:
- Why are you naming one BundleLiteral, but the other VectorExpression? Shouldn't it be VectorLiteral?
- Although we discussed only adding literals, I'm leaning towards adding aggregate expressions. While on the one hand, adding literals is a strict subset (and thus backwards compatible to Chisel users) and could theoretically be added first, it isn't backwards compatible with FIRRTL transform writers who may match on VectorLiteral, and then we would later remove that and add a VectorExpression. I think we need to make a decision about aggregate expressions, and if we are adding them, do it in this PR.
- If we do add aggregate expressions, they must be able to express flipped fields. I'm personally ok this this, but others may have worries.
- Any change to the IR must also have an update to the spec.tex (and update spec.pdf)
Thoughts?
This reflects the fact that it is no longer a literal.
I have switched to doing bundle and vector expressions. So far it seems to work fine for literals and nesting these expressions. I'm currently not testing mixed direction bundle expressions. I'm not really sure what the desired behavior is, nor what the use-cases are (the main one that jumps to mind is assigning a decoupled bundle directly). Do mixed direction vectors make sense? I don't think so... but maybe it does. I think things should "just work™" in the sense that these are pretty thin wrappers and should be mostly equivalent to the expansion that eventually happens. |
|
This is an attempt to add bundle literals to firrtl. I'll make an associated chisel PR.