Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
More Circt intrinsic wrappers (IsX, PlusArgsTest, PlusArgsValue) #2958
More Circt intrinsic wrappers (IsX, PlusArgsTest, PlusArgsValue) #2958
Changes from 18 commits
a47e1d3
89ceba0
8029884
6dbd0f1
f589382
0287aef
483e6e0
32b02d8
ae668da
bd84146
150f7b4
b9cfd3f
f9272ad
9e3c79c
d2e7ca2
ebff9ed
d702109
7f205eb
8ffe833
f22549a
16eb559
94e8c04
dbbf6c0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Code nit. It would likely be cleaner to do the maybe synthesizable unboxing explicitly to share some code:
Alternatively, it would probably be better to have this require a Chisel type and not accept either a Chisel or Hardware type. Ditto for other intrinsics.
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.
yeah we've generally leaned towards making it the user's problem to pass the right thing in (type vs hw)
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 seemed nicer to be able to pass in the kind of thing you are storing the result in rather than having to get it's type yourself. That way you can do a:
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'd say a more common pattern would be
val w = PlusArgsTest(UInt(4.W), "Foo"))
(making wires is uncommon).should we wrap a Default value in this API, if the plus args is not set?
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.
You can directly use
val found = Output(UInt(1.W))
instead of passing this throughFlatIO
. That really only becomes necessary when dealing with aBlackBox
which dynamically checks that there is aval io
.Same nit as above,
Bool
is likely better forfound
.For
result
, is this appropriate to take a type parameter? It seems like we wouldn't know what to do with this if it was anything other than aUInt
? Would this be better expressed as aUInt
of fixed width (for now) or parametric width (later)?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 @seldridge meant
val found = IO(Output(Bool())
(edit mine on the Bool, but the point is you do still need theIO
, but you're allowed to have as manyIO
calls as you want.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.
and we could still take a
gen: UInt
(no type paramterization) letting the user controls the width. Alternatively the old plus arg reader just forced you to specify the width and then it made the UInt with that width, no gen.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.
FlatIO was an old artifact of extmodule. Fixed.
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 take that back, this gives me a bundle for scala purposes but flat io in the module.
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 care about exposing verilog functionality via an intrinsic, not about doing exactly what one code bases blackbox does.
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.
so is the intrinsic handling the unpacking into some complicated data type from the
%d
which is going to just be an integer? Is this useful vs making that happen explicitly in chisel and make this intrinsic just return a UInt?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.
The intrinsic returns any verilog type which can be parsed with format strings (and is supported from firrtl). The format string can be used in verilog to pick apart complex specification strings for subfields you are interested in.
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.
The intrinsic is nothing more than a wrapper for verilog-spec functionality.
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.
right, I am echoing Schuyler's comment above that I'm unclear why this can return anything other than a UInt at this level. Users can easily cast from UInt to their appropriate data type in a safer way than this happening automatically?
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.
The intrinsic isn't doing this, verilog is. What is safer in moving the casting point? It moves unsafe type conversion further into the user code.
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.
Now that I understand that this is meaningful in verilog, I am OK with it. We might want to add something to the scaladoc that in terms of order / unpacking it's equivalent to using a UInt and then .asTypeOf(gen).
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.
Way cleaner than the explicit annotation. 👍
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.
if we are going to support complicated bundle types then please show a test of that here... but I'm not sure the advantage of having it take any Data vs just a UInt (or SInt possibly through some 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.
Here, for example is parsing a non-integer substring. We can't express this in firrtl since we don't have real types, but in verilog:
$value$plusargs("FREQ+%0F",frequency)
you can also control integer encoding in parsing, parse strings, etc.
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 show that being used in the test?
how would we cast something with format string "FREQ+%0F" to a
gen: DecoupledIO(UInt(32.W))
, something the API will happily let us do right now?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.
Example with a struct: #2958 (comment)
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.
and if the format string is user controlled please show some tests/applications of things that are not of the form "SomeSTring=%d"
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.
To what end? The string is passed by chisel verbatim to firrtl. These are not end-to-end verilog generation tests.
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 a user API we are designing. I am trying to understand the utility of exposing this string to the user, as well as the utility of allowing arbitrary gen data types. If we have no use for it, and users are only ever going to reasonably get UInts back, then let's remove the ambiguity int he API. If we are saying "sure, you can format your plusarg as a float, or signed number" then what do we expect the users to assign those to?
Why isn't the utility something like
PlusArgValueUInt
,PlusArgValueSInt
.I'm not even sure what we'd do with a
PlusArgValueFloat
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 guess we are trying to formalize https://github.com/chipsalliance/rocket-chip/blob/master/src/main/resources/vsrc/plusarg_reader.v, which does expose the format string, but not the output data type (which is hard-coded to a UInt of a user-specified width).
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 are not trying to formalize that. We are trying to provide access to functionality described in SV 2012 section 21.6. That that blackbox will be directly implementable is a (intended) side effect.
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.
A missing point is it is not the case that the format has to be "STR=FORMAT", it can be any pattern match.