[FIRRTL][FIRParser] Tokenize Commas

[mmaloney-sf]

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.

[uenoku]

The implementation generally looks good to me! I agree that comma as whitespace is weird and should be fixed but practically I guess the hack improved parser performance quite a bit :) I'm curious how parser performance changes due to increased toke size.

[dtzSiFive]

Technically this is only mandatory as of 4.0.0, chipsalliance/firrtl-spec#191 . However I think just requiring this regardless is fine re:compatibility.

I too am interested in impact of this.

cc chipsalliance/firrtl-spec#188 .

[mmaloney-sf]

@dtzSiFive Thanks for pointing that out. I feel like I removed that verbiage and then forgot it.

And thank you for pointing out that I didn't feature-gate this. I'm not sure how I should proceed on that point. I had plans to add a lot of small tweaks to the syntax, but it could easily make it a bit of spaghetti to keep strict backwards compat. But then again, I don't know much people care about that compatibility.

[seldridge]

Note: no version of Chisel that I have ever seen has ever not emitted commas unconditionally. There should be no risk in changing this and it should be backwards compatible.

[uenoku]

I think parseOptionalInfo needs to be called regardless of RUW.

[darthscsi]

Yea, this both allows a dangling comma and doesn't properly parse optional parts. I think most consumeIf calls I saw are probably wrong.

[mmaloney-sf]

Thanks for the catch, @uenoku.

@darthscsi I'm not sure what you mean. Feel free to add more information. If you have an example that should be added to the parse-errors file, that would be great.

[dtzSiFive]

Thanks for pushing on this! Left some feedback, still going through but wanted to submit that much now.

[darthscsi]

Please add explicit tests that dangling commas in lists and before optional components are rejected.

[mmaloney-sf]

Please add explicit tests that dangling commas in lists and before optional components are rejected.

Done.

[mmaloney-sf]

If there are no other comments, could someone merge?

[uenoku]

LGTM but can we merge after firtool 1.77 release? I don't believe this PR will cause an issue but just in case.

[mmaloney-sf]

Sounds good to me, @uenoku