circt
circt copied to clipboard
[FIRRTL][FIRParser] Tokenize Commas
Due to historical circumstances, the SFC parser for FIRRTL treated commas (,
) as whitespace. This behavior was carried over into the CIRCT implementation of firtool
.
This behavior is utterly bizarre.
Moreover, the FIRRTL spec has indicated comma tokens (,
) for at least every version since last year.
This PR removes this quirk and properly tokenizes commas, requiring them in the places dictated by the FIRRTL spec.
The impact of this PR should be low, but it may break code (in CIRCT, in Chisel, and elsewhere) in places where FIRRTL was being emitted with a loose interpretation, or where the spec was ambiguous.
Changes:
- Update several tests which were leaning into the "commas-are-whitespace" behavior.
- Removed
','
from the list of "horizontal whitespace" and added an explicitFIRToken::comma
token. - Made changes to the various
parse*()
methods to consume the tokens. - Note the use of a pattern I found useful in several places where I use a boolean
first
to skip parsing comma tokens on the first iteration. Please check me here, since I learned C++ before lambdas were a thing. - Made a few judgment calls around unspeced constructs (
smem
). -
- I changed the FIRRTL emitter in one place where the syntax was unspeced, and there seemed to be conflicting examples.
- @seldridge For some reason, it seemed ambiguous whether
layer
decls need commas. I opted for commas. I can swap if I got this backwards. - I added
parseRUW()
as a non-optional variant ofparseOptionalRUW()
, since the comma token can be used to determine optionality in one case.