-
Notifications
You must be signed in to change notification settings - Fork 603
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
Aggregate Literals #921
Aggregate Literals #921
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.
I think conceptually this makes sense, though the implementation could use a bit of cleaning.
@@ -410,8 +410,7 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { | |||
@chiselRuntimeDeprecated | |||
@deprecated("litArg is deprecated, use litOption or litTo*Option", "chisel3.2") | |||
def litArg(): Option[LitArg] = topBindingOpt match { | |||
case Some(ElementLitBinding(litArg)) => Some(litArg) | |||
case Some(BundleLitBinding(litMap)) => None // this API does not support Bundle literals |
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 is intentionally there to note that this API does not and should not support Bundle/Aggregate literals. Code-wise does nothing, it's more documentation.
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.
I'm not sure what you mean. Should litArg not return an AggregateLit
? Regardless, I think it's OK to move the functionality to the LitBinding
unapply, unless you want it explicitly documented here.
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.
It's a legacy function from before the Bundle/Aggregate literal days, so code calling this probably wasn't designed with Bundle literals in mind and some of the more edge cases like partial Bundle literals. So this just takes the sledgehammer approach of doesn't work with new features.
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.
Perhaps similar functionality with a new name should be added. Writing a function that returns a bundle literal is a lot cleaner when you can call .litArg
- look at a9bc2c8 to see how things get uglier when you can't use litArg
.
if (forcedWidth) { | ||
require(widthArg.get >= minWidth, | ||
s"The literal value ${num} was elaborated with a specified width of ${widthArg.get} bits, but at least ${minWidth} bits are required.") | ||
class AggregateLit(val litMap: Seq[(String, Data, LitArg)]) extends LitArg { |
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.
Why Seq instead of Map?
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 you unify the redundant information (you have both the name and a Data, can you just have one) instead of overencoding data and having potential for consistency issues?
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.
I have a Seq
because I would like the order to match the bundle- I think this is important for things like asUInt
to work as expected, but perhaps that is incorrect. I think asUInt
will take a literal like { a : UInt("h3"), b : UInt("h4") }
and use a asUInt
primop rather than emitting the UInt
directly, which would require the order to be correct.
Re: the redundancy, I agree it's not great. I'm not really sure what the right thing to do is, though. I need names to correctly emit firrtl, but the internal chisel stuff wants the Data
. I think it's easier to go from names to data via the bundle API, but I wanted to get your thoughts on that.
I find names somewhat annoying because it seems to be going in a stringly-typed direction.
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.
I think I also thought about names vs. Data ref, and my implementation uses the Data ref. I think when you're emitting, you should reference the top-level Aggregate and get names from there.
I don't think having the Seq for ordering is a good idea, the ordering information then becomes stored in two places (in the Seq ordering and in the Bundle elements ordering) and non-DRY. I think it makes sense to use the Bundle elements as the authoritative data source, with this just providing additional data.
I agree with pawning as much work to FIRRTL as possible - doing things like const prop in the frontend makes it more likely to have subtle differences in behavior. And you have to keep your implementation in sync in two places.
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.
My current approach is that when aggregates are bound as literals, the children get bound as literals too. I think this is a lot more straightforward than references to bundle elements look at topBindingOpt
and then searching through a map. Data
equality checking and hashCode
s don't really work, right?
// Literal binding attached to the root of a Bundle, containing literal values of its children. | ||
case class BundleLitBinding(litMap: Map[Data, LitArg]) extends LitBinding | ||
case class AggregateLitBinding(litArg: AggregateLit) extends LitBinding | ||
object AggregateLitBinding { |
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.
Why have this companion object? Should it be different than ElementLitBinding, which doesn't have this convenience API?
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.
I can write it out every time, but there's only one flavor of AggregateLit
. ElementLitBinding
s have multiple flavors.
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.
I think whatever you choose it should be stylistically consistent with how everything else is done, which currently seems to be no companion object, building the IR *Lit node, and passing that to bindLitArg. Or you can refactor everything if you think that your style is significantly better.
Thanks for the feedback, @ducky64! |
75ec2ff
to
68dd52c
Compare
The tests were using litArg, which shouldn't be done.
This version binds children with literal bindings rather than child bindings. I think it's reasonable that you'd want to treat an element of a bundle literal as a literal, plus I think it's a simpler and more direct way to implement the functionality. I went back to Right now, it isn't too hard to write something that will die in firrtl. More checking should be done- names should be validated, aggregates should check when binding that the litargs have the right type for their fields, etc. The top level API for creating a VecLit doesn't exist. I think the ideal way to implement this is to have an implementation of |
I wrote up my thoughts on bundle literal bindings in #805, but the argument for the current system is basically:
|
Depends on FIRRTL PR |
Stale with too many conflicts, still a good idea though that we really ought to do. |
This (in addition to a FIRRTL PR) is another stab at adding genuine bundle literals. It also broadens the names of things slightly- I think the bindings should probably be on aggregates rather than bundles so Records and Vecs can get in on the party.
The tests are dying in FIRRTL right now, but it seems to be generating FIRRTL that should work. The firrtl it generates should be cleaner, e.g.
should just be
UInt<4>("h0")
.The API is a bit clunky and could definitely be polished more, but I wanted to get feedback on this and firrtl stuff before going to far down that road.
Related issue: This FIRRTL PR and #805
Type of change: feature request
Impact: API modification
Development Phase: implementation