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

Refactor emiter #1879

Merged
merged 3 commits into from
Nov 10, 2020
Merged

Refactor emiter #1879

merged 3 commits into from
Nov 10, 2020

Conversation

sequencer
Copy link
Member

We have more than 1k lines in side Emitter.scala which contains implementations of VerilogEmitter, FirrtlEmitter and base class for general emitter.
These emitters are not only to be used, after #1826 has been merged. A codes clean up is required. So this PR is a proposal to split Emitters to where they should be, and make base of emitter class accessible to new emitter implementations.

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • 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

  • code refactoring

API Impact

Deprecate import these class from firrtl package.
VerilogEmitter
MinimumVerilogEmitter
SystemVerilogEmitter
FirrtlEmitter
ChirrtlEmitter
HighFirrtlEmitter
MiddleFirrtlEmitter
LowFirrtlEmitter
Move implementation specific codes to firrtl.backends.verilog and firrtl.backends.firrtl.
Make class in Emitter.scala not sealed.

Backend Code Generation Impact

None

Desired Merge Strategy

  • Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

Release Notes

move firrtl and verilog emitter to separate packages.

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?

@sequencer sequencer requested a review from a team as a code owner August 31, 2020 07:38
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.

Nice refactor. It makes a ton of sense to break this up.

Couple of thoughts inline. Also, can you clean up what I think are additional usages of the old package. I found the following on something close to master:

# find src/ -type f -name '*.scala' -exec grep 'VerilogEmitter\|FirrtlEmitter' '{}' '+' | grep import
src/main/scala/firrtl/passes/SplitExpressions.scala:import firrtl.{SystemVerilogEmitter, Transform, VerilogEmitter}
src/main/scala/firrtl/stage/phases/AddDefaults.scala:import firrtl.{AnnotationSeq, VerilogEmitter}
src/main/scala/firrtl/stage/transforms/Compiler.scala:import firrtl.{Transform, VerilogEmitter}
src/main/scala/firrtl/Emitter.scala:  import VerilogEmitter._
src/main/scala/firrtl/transforms/formal/RemoveVerificationStatements.scala:import firrtl.{CircuitState, DependencyAPIMigration, MinimumVerilogEmitter, Transform, VerilogEmitter}
src/test/scala/firrtlTests/stage/phases/AddImplicitEmitterSpec.scala:import firrtl.{EmitAllModulesAnnotation, EmitCircuitAnnotation, HighFirrtlEmitter, VerilogCompiler}
src/test/scala/firrtlTests/transforms/BlackBoxSourceHelperSpec.scala:import firrtl.{Transform, VerilogEmitter}
src/test/scala/firrtl/stage/phases/tests/ConvertCompilerAnnotationsSpec.scala:import firrtl.{HighFirrtlCompiler, HighFirrtlEmitter, LowFirrtlCompiler}

src/main/scala/firrtl/Emitter.scala Outdated Show resolved Hide resolved
@sequencer
Copy link
Member Author

any ideas to fix the unidoc? I have no knowledge to it. :(

@jackkoenig
Copy link
Contributor

jackkoenig commented Aug 31, 2020

unidoc is basically just ScalaDoc. To reproduce, run:

sbt +unidoc

From: https://github.com/freechipsproject/firrtl/blob/505fc53338d61acac391d8b04b8bc99fcc92eb69/.travis.yml#L72

Then fix the errors you see. They're just import warnings that ScalaDoc is promoting to errors: https://travis-ci.com/github/freechipsproject/firrtl/jobs/379375482#L828

@sequencer
Copy link
Member Author

oh, got it. fixing now.

@sequencer sequencer force-pushed the refactor_emiter branch 5 times, most recently from 29b0ad1 to f45904c Compare September 7, 2020 05:03
@sequencer
Copy link
Member Author

sequencer commented Sep 8, 2020

done :) @jackkoenig @seldridge

src/main/scala/firrtl/Emitter.scala Outdated Show resolved Hide resolved
src/main/scala/firrtl/Emitter.scala Outdated Show resolved Hide resolved
@sequencer sequencer force-pushed the refactor_emiter branch 2 times, most recently from 34af4b7 to ca6d398 Compare September 29, 2020 15:03
@jackkoenig
Copy link
Contributor

jackkoenig commented Oct 21, 2020

Since this diff is very large, I rebased it on 1.4.x and checked for binary compatibility, there are a few issues:

firrtl: Failed binary compatibility check against edu.berkeley.cs:firrtl_2.12:1.4.0! Found 21 potential problems
 * class firrtl.SystemVerilogEmitter does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.SystemVerilogEmitter")
 * class firrtl.VerilogEmitter does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.VerilogEmitter")
 * method emitter()firrtl.ChirrtlEmitter in class firrtl.NoneCompiler has a different result type in current version, where it is firrtl.backends.firrtl.ChirrtlEmitter rather than firrtl.ChirrtlEmitter
   filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("firrtl.NoneCompiler.emitter")
 * method emitter()firrtl.MiddleFirrtlEmitter in class firrtl.MiddleFirrtlCompiler has a different result type in current version, where it is firrtl.backends.firrtl.MiddleFirrtlEmitter rather than firrtl.MiddleFirrtlEmitter
   filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("firrtl.MiddleFirrtlCompiler.emitter")
 * method emitter()firrtl.HighFirrtlEmitter in class firrtl.HighFirrtlCompiler has a different result type in current version, where it is firrtl.backends.firrtl.HighFirrtlEmitter rather than firrtl.HighFirrtlEmitter
   filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("firrtl.HighFirrtlCompiler.emitter")
 * class firrtl.HighFirrtlEmitter does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.HighFirrtlEmitter")
 * method emitter()firrtl.MinimumVerilogEmitter in class firrtl.MinimumVerilogCompiler has a different result type in current version, where it is firrtl.backends.verilog.MinimumVerilogEmitter rather than firrtl.MinimumVerilogEmitter
   filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("firrtl.MinimumVerilogCompiler.emitter")
 * class firrtl.VerilogEmitter#EmissionOptions does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.VerilogEmitter$EmissionOptions")
 * class firrtl.FirrtlEmitter does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.FirrtlEmitter")
 * class firrtl.VerilogEmitter#VerilogRender does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.VerilogEmitter$VerilogRender")
 * class firrtl.VerilogEmitter#EmissionOptionMap does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.VerilogEmitter$EmissionOptionMap")
 * class firrtl.LowFirrtlEmitter does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.LowFirrtlEmitter")
 * method emitter()firrtl.SystemVerilogEmitter in class firrtl.SystemVerilogCompiler has a different result type in current version, where it is firrtl.backends.verilog.SystemVerilogEmitter rather than firrtl.SystemVerilogEmitter
   filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("firrtl.SystemVerilogCompiler.emitter")
 * method emitter()firrtl.VerilogEmitter in class firrtl.VerilogCompiler has a different result type in current version, where it is firrtl.backends.verilog.VerilogEmitter rather than firrtl.VerilogEmitter
   filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("firrtl.VerilogCompiler.emitter")
 * object firrtl.VRandom does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.VRandom$")
 * class firrtl.ChirrtlEmitter does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.ChirrtlEmitter")
 * class firrtl.VRandom does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.VRandom")
 * object firrtl.VerilogEmitter does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.VerilogEmitter$")
 * class firrtl.MiddleFirrtlEmitter does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.MiddleFirrtlEmitter")
 * class firrtl.MinimumVerilogEmitter does not have a correspondent in current version
   filter with: ProblemFilters.exclude[MissingClassProblem]("firrtl.MinimumVerilogEmitter")
 * method emitter()firrtl.LowFirrtlEmitter in class firrtl.LowFirrtlCompiler has a different result type in current version, where it is firrtl.backends.firrtl.LowFirrtlEmitter rather than firrtl.LowFirrtlEmitter

Looks like mainly some deprecated aliases are needed.

@sequencer sequencer force-pushed the refactor_emiter branch 2 times, most recently from 655ce00 to f79d9f4 Compare November 2, 2020 17:36
@sequencer
Copy link
Member Author

I added the deprecated aliases, but still have these error, I think it can be ignored?

@sequencer sequencer force-pushed the refactor_emiter branch 3 times, most recently from 694df88 to 51bab16 Compare November 3, 2020 17:12
@sequencer
Copy link
Member Author

Remove all incompatible stuff, didn't change the package name, only split file now.
package moving and type alias are binary incompatible, will exist in my next PR.

Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

Confirmed this is binary compatible. Thanks @sequencer!

@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Nov 9, 2020
@mergify mergify bot merged commit 92af63c into master Nov 10, 2020
mergify bot pushed a commit that referenced this pull request Nov 10, 2020
* split big Emitter to submodules.

* fix all deprecated warning.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 92af63c)
@mergify mergify bot mentioned this pull request Nov 10, 2020
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label Nov 10, 2020
mergify bot added a commit that referenced this pull request Nov 10, 2020
* split big Emitter to submodules.

* fix all deprecated warning.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 92af63c)

Co-authored-by: Jiuyang Liu <[email protected]>
@jackkoenig jackkoenig deleted the refactor_emiter branch December 2, 2020 18:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Backported This PR has been backported to marked stable branch Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants