Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions revision-history.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ revisionHistory:
# additions to the specification should append entries here.
thisVersion:
- Specify behavior of zero bit width integers, add zero-width literals
- Add bit-index assignment and syntax
# Information about the old versions. This should be static.
oldVersions:
- version: 1.1.0
Expand Down
53 changes: 53 additions & 0 deletions spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -990,6 +990,9 @@ sub-element in the vector.
Invalidating a component with a bundle type recursively invalidates each
sub-element in the bundle.

Invalidating a particular subset of bits in an integer is possible by
Copy link
Collaborator

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?

performing a bit-index with `is invalid`{.firrtl} (see [@sec:bit-indices]).

## Attaches

The `attach`{.firrtl} statement is used to attach two or more analog signals,
Expand Down Expand Up @@ -1840,6 +1843,49 @@ module MyModule :
out[4] <= in
```

## Bit-indices

The bit-index expression statically refers, by index, to a particular bit, or
slice of bits, of an expression with an integer type (`UInt`{.firrtl} or
`SInt`{.firrtl}). The indices must be non-negative integers and cannot be equal
to or exceed the width of the integer they index. The bit-index `x[hi:lo]`
selects bits `hi` (most significant) through `lo` (least significant) of `x`.
The bit-index `x[i]` selects the single bit `i`.

The type of the bit-index expression `x[hi:lo]`.{firrtl} is `UInt<hi - lo +
1>`.{firrtl} (even if `x` is an `SInt`) and the type of `x[i]`.{firrtl} is
`UInt<1>`.{firrtl}. This means that when connecting to a bit-indexed value, the
right-hand-side of the connection must be a `UInt`.{firrtl}, even if the value
being indexed is an `SInt`.{firrtl}.

The bit-index can be used as a sink or source. When used as a source,
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very much opposed to introducing duplicate functionality since it will make the compiler more complicated for little to no gain.

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.

Copy link
Contributor

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.

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.

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.

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.

Copy link
Member

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.

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.

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.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 from bits to [...].

`x[hi:lo]` is equivalent to `bits(x, hi, lo)` and `x[i]` is equivalent to
`bits(x, i, i)`. See the `bits`{.firrtl} primitive operation
([@sec:bit-extraction-operation]). When used as a sink, the bit-index assigns
to only the sliced bits of the integer. If a value has multiple bit-index
assignments, the assignments are accumulated in order according to last-connect
semantics, in the same way as the behavior of last-connect semantics for
aggregate types (see [@sec:last-connect-semantics]).
zyedidia marked this conversation as resolved.
Show resolved Hide resolved

A value that is bit-indexed must be fully initialized at the bit-level. There
must be a valid assignment accounting for every bit in the value. Registers are
implicitly initialized with their current contents.
Copy link
Collaborator

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.


Bit-indexing does not participate in width inference (see
[@sec:width-inference]), and if a bit-index is applied to a value with an
unspecified width, that value must have another use that allows its width to be
inferred. Otherwise this causes an error.
Copy link
Collaborator

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?

Copy link
Author

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/when bits 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.


The following example connects the `in`{.firrtl} port to the fifth bit
of the `out`{.firrtl} port.

``` firrtl
module MyModule :
input in: UInt<1>
output out: UInt<10>
out[4] <= in
zyedidia marked this conversation as resolved.
Show resolved Hide resolved
```

## Sub-accesses

The sub-access expression dynamically refers to a sub-element of a vector-typed
Expand Down Expand Up @@ -2382,6 +2428,8 @@ wire or register is duplex.
The flow of a sub-index or sub-access expression is the flow of the vector-typed
expression it indexes or accesses.

The flow of a bit-index is the flow of the integer-typed expression it indexes.

The flow of a sub-field expression depends upon the orientation of the field. If
the field is not flipped, its flow is the same flow as the bundle-typed
expression it selects its field from. If the field is flipped, then its flow is
Expand Down Expand Up @@ -2413,6 +2461,8 @@ expression.

The width of each primitive operation is detailed in [@sec:primitive-operations].

The width of a bit-index is detailed in [@sec:bit-indices].

The width of the integer literal expressions is detailed in their respective
sections.

Expand Down Expand Up @@ -2626,6 +2676,8 @@ following restrictions:

- The dynamic sub-access expression is not used.

- The bit-index expression is not used as a sink.

- All components are connected to exactly once.

- All uninferred `Reset`{.firrtl} types have been inferred to `UInt<1>`{.firrtl}
Expand Down Expand Up @@ -2896,6 +2948,7 @@ expr =
reference = id
| reference , "." , id
| reference , "[" , int , "]"
| reference , "[" , int , ":" , int , "]"
| reference , "[" , expr , "]" ;

(* Memory *)
Expand Down