Skip to content

Conversation

jackkoenig
Copy link
Contributor

Enables #4644

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • 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 add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • API modification

Desired Merge Strategy

  • Squash

Release Notes

  • Remove ChiselAnnotation and ChiselMultiAnnotation
  • Remove annotate methods for ChiselAnnotations
  • Remove Circuit APIs from ChiselOptions and ChiselCircuitAnnotation
    • Use ElaboratedCircuit instead
  • Remove ChiselEnum related annotations

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash) and clean up the commit message.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@jackkoenig jackkoenig added this to the 7.0 milestone Feb 21, 2025
@jackkoenig jackkoenig requested a review from seldridge February 21, 2025 23:54
@jackkoenig jackkoenig force-pushed the jackkoenig/remove-legacy-annotations branch from 1a043e7 to 1a7dbe2 Compare February 22, 2025 00:48
@jackkoenig jackkoenig mentioned this pull request Feb 22, 2025
14 tasks
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.

LGTM

Comment on lines -616 to +615
def annotations: ArrayBuffer[ChiselAnnotation] = dynamicContext.annotations
def annotations: ArrayBuffer[() => Seq[Annotation]] = dynamicContext.annotations
Copy link
Member

Choose a reason for hiding this comment

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

At a very basic level, this kind of cleanup is great. Previously, we were using a class that was wrapping a function. Now we are directly using a function. 💯

annotations.foreach(annotate(_))
// annotate doesn't support InstanceId (which is deprecated) because InstanceId doesn't implement toRelativeTarget
// this API is deprecated anyway so probably fine to not check it.
annotate()(Seq(SinkAnnotation(component.toNamed, name)) ++ Option.when(disableDedup)(NoDedupAnnotation(moduleName)))
Copy link
Member

Choose a reason for hiding this comment

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

That seems okay to me. Also, given that this is deprecated since 6.0, delete in a follow-on? 💣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was exactly my thinking, I just figured it should be a separate PR

* Remove ChiselAnnotation and ChiselMultiAnnotation
* Remove annotate methods for ChiselAnnotations
* Remove Circuit APIs from ChiselOptions and ChiselCircuitAnnotation
  * Use ElaboratedCircuit instead
* Remove ChiselEnum related annotations
@jackkoenig jackkoenig force-pushed the jackkoenig/remove-legacy-annotations branch from 1a7dbe2 to c5f0b60 Compare February 24, 2025 20:17
@jackkoenig jackkoenig enabled auto-merge (squash) February 24, 2025 20:38
@jackkoenig jackkoenig merged commit c1643b7 into main Feb 24, 2025
14 checks passed
@jackkoenig jackkoenig deleted the jackkoenig/remove-legacy-annotations branch February 24, 2025 20:40
tymcauley added a commit to tymcauley/rocket-chip that referenced this pull request Mar 7, 2025
This feature is being removed from Chisel 7:
chipsalliance/chisel#4717

These custom annotations aren't used by `firtool`, so their removal
doesn't impact that tool's outputs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants