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

Deprecate CompilerAnnotation #1870

Merged
merged 7 commits into from
Aug 28, 2020
Merged

Deprecate CompilerAnnotation #1870

merged 7 commits into from
Aug 28, 2020

Conversation

seldridge
Copy link
Member

@seldridge seldridge commented Aug 27, 2020

This is a follow-on to #1835 to fully deprecate CompilerAnnotation.

This does a couple of things:

  1. The CompilerAnnotation$ companion object is changed to emit RunFirrtlTransformAnnotations instead of CompilerAnnotations. In effect, when you do -X verilog, you now get a RunFirrtlTransformAnnotation(new VerilogEmitter) as opposed to a CompilerAnnotation(new VerilogCompiler).

  2. This adds a ConvertCompilerAnnotation phase which is doing an explicit conversion of a CompilerAnnotation to a RunFirrtlTransformAnnotation. This differs from stage: allow a RunFirrtlTransformAnnotation(_:Emitter) annotation to be used in place of a CompilerAnnotation #1835 which relaxed support so that a RunFirrtlTransformAnnotation(_ <: Emitter) could substitute for a CompilerAnnotation. This PR goes one step further and forces internal phases to only understand transforms.

  3. Better checks and warnings are now provided inside ConvertCompilerAnnotation. The check of a single compiler is migrated from Checks here. Additionally, if a CompilerAnnotation is converted, a verbose warning is printed telling you exactly how to replace your transform, e.g., you now get:

CompilerAnnotation is deprecated since FIRRTL 1.4.0. Please use 'RunFirrtlTransformAnnotation(new firrtl.HighFirrtlEmitter)' instead.

Note that you should only now get a warning if you actually use a CompilerAnnotation. Any use of -X will not produce an error.

Contributor Checklist

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

  • bug fix
  • code refactoring

API Impact

None. Removes a warning that the user couldn't do anything to fix.

Backend Code Generation Impact

None.

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

None.

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?

Change the CompilerAnnotation object to emit
RunFirrtlTransformAnnotations containing the associated emitter.

This requires a fix in the Driver compatibility layer to know how to
enable one-file-per module emission if either a CompilerAnnotation or
a RunFirrtlTransformAnnotation(_: Emitter) is present.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
Add a phase, ConvertCompilerAnnotation, that converts a
CompilerAnnotation to a RunFirrtlTransformAnnotation. This provides a
warning to the user if this path is taken.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
@seldridge seldridge added this to the 1.4.0 milestone Aug 27, 2020
@seldridge seldridge requested a review from a team as a code owner August 27, 2020 23:52
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.

This fixes the annoying deprecation warning when running (new ChiselStage).emitChirrtl (or whatever) and looks good. A few minor questions but the overall PR looks great.

Deprecate the CompilerAnnotation companion object and move it's
private utility inside the RunFirrtlTransformAnnotation companion
object.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
Make this phase private to avoid adding a deprecation warning. Also,
remove an unused string value.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
@ekiwi
Copy link
Contributor

ekiwi commented Aug 28, 2020

Did you make sure that specifying only -X without -E works? What stage will add the EmitCircuitAnnotation?

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
@seldridge
Copy link
Member Author

@ekiwi: firrtl.stage.phases.AddImplicitEmitter is what is taking care of this. Fortunately, your edits in your original PR are already handling this case nicely.

I added a test to FirrtlMainSpec to hit this in a follow-up commit.

Copy link
Contributor

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

:shipit:

@ekiwi ekiwi added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Aug 28, 2020
@mergify mergify bot merged commit a8d7b9a into master Aug 28, 2020
@jackkoenig jackkoenig deleted the fix-compiler-annotation branch March 26, 2021 23:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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