Specification syntax examples rejected by current parser implementation.

[ben-marshall]

Type of issue: bug report

• If possible, please directly provide the FIRRTL file or a relevant fragment if the file is large

circuit myCircuit:                        
    module myCircuit:                     
        input  clk: Clock                 
        input  k  : UInt<1>               
        output c  : UInt<1>               
        when k : (c <= k) else : (c <= k) 

• What is the current behavior?

ben@benm-lt:~/tools/firrtl/syntax-tests$ ../firrtl/utils/bin/firrtl  -i circuit2.fir -o output.v
line 6:17 mismatched input '(' expecting {':', '[', '.'}
Exception in thread "main" firrtl.SyntaxErrorsException: 1 syntax error(s) detected
	at firrtl.Parser$.$anonfun$parseCharStream$1(Parser.scala:45)
	at firrtl.Utils$.time(Utils.scala:182)
	at firrtl.Parser$.parseCharStream(Parser.scala:33)
	at firrtl.Parser$.parseFile(Parser.scala:25)
	at firrtl.Driver$.$anonfun$getCircuit$5(Driver.scala:199)
	at scala.Option.getOrElse(Option.scala:121)
	at firrtl.Driver$.$anonfun$getCircuit$3(Driver.scala:182)
	at scala.Option.getOrElse(Option.scala:121)
	at firrtl.Driver$.$anonfun$getCircuit$1(Driver.scala:182)
	at scala.util.Try$.apply(Try.scala:209)
	at firrtl.Driver$.getCircuit(Driver.scala:170)
	at firrtl.Driver$.$anonfun$execute$1(Driver.scala:226)
	at logger.Logger$.$anonfun$makeScope$2(Logger.scala:138)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at logger.Logger$.makeScope(Logger.scala:136)
	at firrtl.Driver$.execute(Driver.scala:223)
	at firrtl.Driver$.execute(Driver.scala:301)
	at firrtl.Driver$.main(Driver.scala:317)
	at firrtl.Driver.main(Driver.scala)

• What is the expected behavior?

According to the current specification document in the repository, line 6 of the example is valid syntax. See section 12, page 51 - "Details about syntax" where the spec says that

when c:
    a <= b
else:
    c <= d
    e <= f

can be equivalently expressed on a single line as follows.

when c : (a <= b) else : (c <= d, e <= f)

• Please tell us about your environment:
  • version: 860e6844708e4b87ced04bcef0eda7810cba106a (HEAD As of Date: Thu Sep 13 22:09:18 2018 -0700)

• Linux benm-lt 4.15.0-33-generic 36-Ubuntu SMP Wed Aug 15 16:00:05 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

What is the use case for changing the behavior?
The parser does not match the specification. Other implementations of an FIRRTL parser will behave differently.

Impact: unknown

Development Phase: request

Other Information
I have played with lots of toy examples for FIRRTL syntax with the aim of building my own parser for it. However, the current specification does not match the implementation so I can't progress. It looks like the current implementation does not support any of the one-line syntax correctly, only the block/indent syntax. Is this expected?

There are a bunch of ambiguities in the syntax specification as well, such as whether one can break out one one-line syntax and into indented syntax. One line syntax with commas is also completely absent from the concrete syntax tree. I don't know whether this or a new issue is the best place to discuss these problems?

Cheers,
Ben

[azidar]

Yeah, we gave up supporting the inline syntax, but haven't updated the documentation. You can go ahead and continue your implementation assuming chisel won't generate any inline syntax. I'm not sure I have time at the moment to update the spec, but I'd certainly merge a PR if you were willing to contribute! What language are you writing the parser for?

[ben-marshall]

Yeah, we gave up supporting the inline syntax, but haven't updated the documentation.

Oh wow. Okay, glad I asked!

You can go ahead and continue your implementation assuming chisel won't generate any inline syntax.

Will do. Is there anything else I should know? I don't trust the specification now for obvious reasons.

I'm not sure I have time at the moment to update the spec, but I'd certainly merge a PR if you were willing to contribute!

I'm very happy to contribute. I'm short on time as well, but a lot of my future work will depend on tools like this, so it's a good investment. I can try and re-write the spec to remove references to the in-line syntax, but it will need very thorough review by yourself or some other member of the chissel developers.

Any change will require corrections to the concrete syntax specification as well, since it is missing INDENT/DEDENT tokens and the explicit newlines needed to delimit statements.

Specifications are the most important part of projects like this, so keeping them up to date has got to be a priority, but I'm not necessarily the most qualified to do it. I'll see how I go.

What language are you writing the parser for?

I haven't decided yet. Its very much a hobby project until I get it to a usable state. Probably something like Python.

[jackkoenig]

Specifications are the most important part of projects like this, so keeping them up to date has got to be a priority

We definitely need to do a better job of it

Probably something like Python

Have you thought about using ANTLR to generate a Python parser? Support for Java, Python, and C++ are why we originally used ANTLR as a parser generator.

I should note that while we still use the Parser, for performance reasons (primarily memory use) we have been moving toward a Protocol Buffer message-based implementation. See .proto file. This information also needs to be added to the spec.

[ben-marshall]

We'll, I've forked FIRRTL and have a wierd interest in specifications. So I'll have a go at removing references to the inline syntax and updating what the current parser does (using your current ANTLR one as a golden reference). I'll reference this issue in the pull request as well.

Have you thought about using ANTLR to generate a Python parser?

Yes that is one option I've been considering. The upside being that the set of acceptable inputs to the parser is guaranteed to be identical to the SCALA implementation.

The other option was a clean-room implementation from the spec using the lark-parser which is a really nice (IMHO) pure python library. The advantage there being that it is all python, and there is no need to ship around so much generated code.

I think that given this discussion, ANTLR will be the way to go. The most important thing for any FIRRTL parser will be "if it parses for you it will also parse for me".

we have been moving toward a Protocol Buffer message-based implementation

That's interesting! I've not come across that before. Is it simply a fast way to stream data from say the Chissel back-end into an FIRRTL front-end without going via a file? I can see from the spec that FIRRTL will be a pretty verbose way of expressing even medium size designs, though it is very ammenable to compression if you consider a binary file format. That's probably getting very far ahead though ;)

[jackkoenig]

The other option was a clean-room implementation from the spec using the lark-parser which is a really nice (IMHO) pure python library. The advantage there being that it is all python, and there is no need to ship around so much generated code.

I think that given this discussion, ANTLR will be the way to go. The most important thing for any FIRRTL parser will be "if it parses for you it will also parse for me".

That library does look really slick. I definitely think that using the ANTLR library as a starting point is the right way to go and a much lower barrier to entry. Perhaps some day it would make sense to migrate to that but at least in the near term it should be fairly straight forward to get stuff working using ANTLR!

That's interesting! I've not come across that before. Is it simply a fast way to stream data from say the Chisel back-end into an FIRRTL front-end without going via a file? I can see from the spec that FIRRTL will be a pretty verbose way of expressing even medium size designs, though it is very ammenable to compression if you consider a binary file format. That's probably getting very far ahead though ;)

It's kind of orthogonal from whether or not a file is involved. For example, chisel3.Driver.execute used to emit the circuit to a String that FIRRTL would then parse. Now we walk Chisel's internal datastructure and directly convert it to FIRRTL's AST, no parsing or ProtoBuf involved :)

We decided to use ProtoBuf because it was suggested and it seems to have a lot of advantages over other (de)serialization formats like speed, making it easy (and default behavior) to maintain forward and backwards compatibility, etc. The straw that broke the camels back though was the ANTLR parser has some performance cliff for very large designs that I wasn't able to debug. I don't think that's something for you to worry about at the moment, but just be aware.