circt icon indicating copy to clipboard operation
circt copied to clipboard

[FIRRTL][FIRParser] Tokenize Commas

Open mmaloney-sf opened this issue 8 months ago • 3 comments

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 explicit FIRToken::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 of parseOptionalRUW(), since the comma token can be used to determine optionality in one case.

mmaloney-sf avatar Jun 19 '24 07:06 mmaloney-sf