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
[minor] Bit-index support (subword assignment) #26
base: main
Are you sure you want to change the base?
[minor] Bit-index support (subword assignment) #26
Changes from 11 commits
51a68cf
86bcca2
b2dff2e
30c6c30
c6e26cd
2e26a19
65ebef3
267e1df
2872b62
eb59122
c72b34d
8e077c7
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.
This is starting to sound like invalid is being used for don't care.
For normal wires/regs/outputs, how does invalidating a subset of bits get you anything that starting with an entire invalid value which is later partially written not get you?
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.
Have you considered restricting your proposal to L-value bit slices?
All R-value uses seem to already be covered by
bits
and thus it might help to focus the proposal on sub-word assignments.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 true that if it isn't restricted to L-value slices there is redundancy with the
bits
primop, but I think the intention is to move to a new syntax that is consistent for both L and R-values, and phase out the bits primop in the future. This also has the benefit of making the Chisel emission simpler, since it doesn't have to emit different syntax based on whether the index is an L-value or an R-value.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 don't think that is necessary. As I said, firrtl is an IR, so it can be more explicit about some things than a user-facing language. I am very much opposed to introducing duplicate functionality since it will make the compiler more complicated for little to no gain.
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 clarify: the plan is to remove
bits
for this exact reason and replace it with bit-index.Is the concern about atomicity of updates to the spec? I was planning to just remove
bits
in a separate PR in favor of bit index.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 not keep
bits
? As @darthscsi pointed out above,bits
is easier to parse and does not conflict with sub-access. Otherwise to distinguish sub-access and bit-index, one would have to know the type of the expression.Yes. I believe that before merging into
main
, the complete proposal should be considered.In addition to that, removing
bits
will make for a back-wards incompatible change and I do not see a good reason for this, when we could just stick with the old syntax and either extend it to l-values or come up with an alternative syntax specifically for l-values. The handling of l-value and r-value bit-index is quite different anyways.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 seems clean to have a single unified op for extraction.
FIRRTL really screwed up here with not adding type information to each operation. Any sane parser is going to track the types of references and uses that to build up its internal FIRRTL IR (which necessarily must include type information). Hence, I'm not super concerned about this. I do admit that this means
foo[a] <= bar
is ambiguous without extra context. However,foo[a] <= bar : uint<8>, uint<1>
is not and would be a great direction to that the FIRRTL textual format.I'm fine to just remove
bits
entirely here then.I don't know if backwards compatibility should be a goal here. We're attempting to make it easy for FIRRTL compilers to check if they support a given FIRRTL text via: #30. I guess my concern is that it seems weird to try to be backwards compatible for bits on the RHS of a connect when the fundamental change is to extend the spec in an entirely backwards incompatible way. Or: SFC will be "incompatible" with FIRRTL 2.0.0+ after this change even though it will work for Chisel designs where a user doesn't use bit index.
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 main point is that it will be harder to support older FIRRTL versions with the same compiler if we make this change. If we only add a new operation, then we trivially support older FIRRTL versions. However, if we replace
bits
with[..]
, then we will still have to keep around the old code to support older FIRRTL versions. Not an insurmountable problem for sure. But there also does not seem to be a real upside to switching frombits
to[...]
.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 redundant with the section on registers.
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 not say that a bit-index forces the width to be at least sufficient to the slice bounds?
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 that is definitely desirable, but I think the motivation for not participating in width inference was so that the the bit-index would be interchangeable with the
bits
primop (when used as an R-value), and then if/whenbits
gets removed the width inference behavior you describe could be added.If we want to add width inference support as part of this proposal, then perhaps this proposal should also add width inference for the bits primop.
I think either approach is reasonable.