Skip to content
This repository has been archived by the owner on Aug 20, 2024. It is now read-only.

Improve ANTLR Parser #2343

Merged
merged 4 commits into from
Dec 1, 2021
Merged

Improve ANTLR Parser #2343

merged 4 commits into from
Dec 1, 2021

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Aug 31, 2021

2 Commits for 2 changes:

  1. Factor references into their own parse rules and remove left-recursion
  2. Add parser Listener that turns CST into AST on the fly and nulls out CST to save memory

Performance Enhancement

Time required to parse and emit CHIRRTL

design master (.fir) this PR (.fir) .pb on both
small 5.3 s 5.3 s 2.5 s
medium 35 s 33 s 12.7 s
large 170 s 124 s 41 s

Note a huge gain for performance, but a wash or strict improvement. Still much slower than .pb parsing but could probably be helped by more (or any) parallelism.

Memory Use Enhancement

Required heap to parse and emit CHIRRTL

design master (.fir) this PR (.fir) .pb on both
small 800 M 500 M 300 M
medium 9 G 4.6 G 3.4 G
large 42 G 13.5 G 12 G

Still not quite as good as .pb but a massive improvement. In profiling, the main objects being created in parsing are refs and subrefs, so perhaps we tweak the parsing rules again to not fully parse them and maybe parse them as part of a 2nd phase / during the CST => AST step?

Contributor Checklist

  • [None] Did you add Scaladoc to every public function/method?
  • [NA] Did you update the FIRRTL spec to include every new feature/behavior?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • performance improvement
  • new feature/API

API Impact

Technically the generated Listener code (not the one I added, but the stuff generated by ANTLR) is public. I don't think it should be considered part of the FIRRTL public API but it's possible people would use it directly. We should probably move these files to package firrtl.internal.antlr or something to stop users from using them.

Backend Code Generation Impact

No impact

Desired Merge Strategy

  • Rebase

Release Notes

Improve performance and vastly reduce memory usage of .fir parser

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@jackkoenig jackkoenig force-pushed the improve-parser branch 2 times, most recently from e9c9393 to 28d687a Compare December 1, 2021 18:55
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with anything here as long as it passes regressions. Thanks for hacking on parsing time, Jack!

@jackkoenig
Copy link
Contributor Author

The failures are just build issues in the formal equivalence flow, not real failures, figuring out a fix.

The classes should not really be part of the firrtl public API to begin
with and they cause issues during ScalaDoc generation.
Tweak the grammar to handle references without left-recursion. Also
split references and subreferences out from the regular expression rule
to make their parsing more efficient.
The ANTLR-generated concrete syntax tree (CST) takes up much more memory
than the parsed .fir file. By using a Listener, we can construct the
FIRRTL AST live with CST construction and null out the CST as we consume
pieces of it. Not only does this improve performance, it drastically
reduces max memory use for the parser.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants