chipsalliance / firrtl-spec

The specification for the FIRRTL language
45 stars 28 forks source link

Does the firrtl spec support splitting long lines? #4

Open madebr opened 4 years ago

madebr commented 4 years ago

This is not a bug report per se, but more of a question. See listing 15 from the specs:

{real: {word:UInt<32>, valid:UInt<1>, flip ready:UInt<1>}
imag: {word:UInt<32>, valid:UInt<1>, flip ready:UInt<1>}}

The bundle is split among multiple lines with indentation. The same goes for listing 54, where the type of the mem block is shown.

Does the firrtl spec support splitting long code among multiple lines, or is this a result of formatting?

This is a bug report after all: I think there should be a comma after between the real and imag bundle.

seldridge commented 2 years ago

I fixed the missing comma issue in d169da3adea4686bee4287357c109820d2116ec0. I believe this existed in the spec only to work around long-line issues. However, it makes sense to me to allow this as long as it's unambiguous. E.g., for types, once you're opened a scope with : or {, newlines don't matter and you can add them however you want.

This is slightly complicated because the Scala-based FIRRTL Compiler (SFC) will reject these while the MLIR-based FIRRTL Compiler (MFC) will accept these. Specifically, the following shows this delineation:

circuit Foo:
  module Foo:
    input a: {a: UInt<1>,
              b: UInt<1>}
    output b: {a: UInt<1>,
               b: UInt<1>}

    b <= a

The SFC antlr grammar is complaining about unexpected newlines or indents while MFC doesn't care. After it sees a { it is in "bundle parsing mode" and will parse anything it sees ignoring newlines or whitespace.

My reasoning for why this should be allowed is that other indentation-sensitive languages (e.g., Python) allow this. Specifically, Python lets you do things like the following:

if True:
    print("hello" +
"world")

if True:
    print("hello" +
          "world")

if True:
    print("hello" +

                   "world")