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 21 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.
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.
The PR can do a
takeWhile(_ != '@')
to strip the source locators before the trim. (Generally it's bad to match on the source locator.)