Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add safer Chisel annotation API, deprecate old ones (backport #4643) #4697

Merged
merged 4 commits into from
Feb 15, 2025

Conversation

chiselbot
Copy link
Collaborator

Note that I tested that the deprecated versions of annotate work by updating them first, making sure all tests pass, then removing all uses of ChiselAnnotation and ChiselMultiAnnotation except in ChiselEnum. The ones in ChiselEnum have duplication checking that I could replicate with the annotations, but is annoying. I figure we'd rather just remove those (and roll out firrtl enums? 🙏) so I just left them. They also do help test the deprecated APIs.

This PR has two purposes:

  1. Make it possible to check that any data being annotated are annotatable. You can see we've done that for some annotations but not all--this new API enforces it for all annotations (or at least encourages enforcement).
  2. By [softly] requiring the user to record what Data are being annotated up front, we can improve the view renaming logic. Rather than having to conservatively rename tons and tons of views, we only do so for those that are actually annotated. This should be a huge performance improvement for designs that use lots of views.

(2) will be in a follow on PR.

One weirdness in the PR that I would appreciate review of is that the new API asks the caller to "report" all InstanceIds that will be annotated. We really only currently care about Data but I figure in the future we may want checks for other things. The awkwardness though, is that InstanceId includes Data, BaseModule, and MemBase, but not Hierarchy nor the new SRAM targets. So if we wanted more checks, we might need to change the API anyway in the future since users can't currently report Hierarchy or SRAM targets being annotated. I could take a step back and try to find a way to unify all of these, or just leave it, or switch the API to Data to make it more clear that that is all we're checking. WDYT?

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 deprecation

Desired Merge Strategy

  • Squash

Release Notes

Creating annotations in Chisel now requires reporting what InstanceIds are going to be annotated so that Chisel can do some safety checks.

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), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

This is an automatic backport of pull request #4643 done by [Mergify](https://mergify.com).

The new one enables safety checks and smarter logic for views.

(cherry picked from commit a95cfe4)

# Conflicts:
#	build.mill
#	build.sbt
#	core/src/main/scala/chisel3/Annotation.scala
#	core/src/main/scala/chisel3/ChiselEnum.scala
#	core/src/main/scala/chisel3/Module.scala
#	core/src/main/scala/chisel3/dontTouch.scala
#	core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala
#	src/main/scala/chisel3/util/AttributeAnnotation.scala
#	src/main/scala/chisel3/util/experimental/Inline.scala
#	src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala
#	src/main/scala/circt/OutputDirAnnotation.scala
#	src/test/scala-2/chiselTests/util/SRAMSpec.scala
#	src/test/scala/chiselTests/AnnotatingDiamondSpec.scala
#	src/test/scala/chiselTests/experimental/hierarchy/Annotations.scala
#	src/test/scala/circtTests/stage/ChiselStageSpec.scala
Copy link
Contributor

mergify bot commented Feb 14, 2025

Cherry-pick of a95cfe4 has failed:

On branch mergify/bp/6.x/pr-4643
Your branch is up to date with 'origin/6.x'.

You are currently cherry-picking commit a95cfe4c.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   core/src/main/scala/chisel3/experimental/EnumAnnotations.scala
	new file:   core/src/main/scala/chisel3/experimental/Targetable.scala
	modified:   core/src/main/scala/chisel3/experimental/Trace.scala
	modified:   core/src/main/scala/chisel3/internal/Builder.scala
	modified:   core/src/main/scala/chisel3/internal/firrtl/IR.scala
	modified:   core/src/main/scala/chisel3/package.scala
	modified:   docs/src/cookbooks/hierarchy.md
	modified:   src/main/scala/chisel3/util/BlackBoxUtils.scala
	modified:   src/main/scala/chisel3/util/ExtModuleUtils.scala
	modified:   src/main/scala/chisel3/util/experimental/ForceNames.scala
	modified:   src/main/scala/chisel3/util/experimental/decode/decoder.scala
	modified:   src/main/scala/circt/Convention.scala
	modified:   src/test/scala/chiselTests/BoringUtilsSpec.scala
	modified:   src/test/scala/chiselTests/ChiselEnum.scala
	modified:   src/test/scala/chiselTests/NewAnnotationsSpec.scala
	modified:   src/test/scala/chiselTests/experimental/DataViewTargetSpec.scala
	modified:   src/test/scala/chiselTests/stage/phases/ConvertSpec.scala

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   build.mill
	both modified:   build.sbt
	both modified:   core/src/main/scala/chisel3/Annotation.scala
	both modified:   core/src/main/scala/chisel3/ChiselEnum.scala
	both modified:   core/src/main/scala/chisel3/Module.scala
	both modified:   core/src/main/scala/chisel3/dontTouch.scala
	both modified:   core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala
	deleted by us:   src/main/scala/chisel3/util/AttributeAnnotation.scala
	both modified:   src/main/scala/chisel3/util/experimental/Inline.scala
	both modified:   src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala
	deleted by us:   src/main/scala/circt/OutputDirAnnotation.scala
	deleted by us:   src/test/scala-2/chiselTests/util/SRAMSpec.scala
	both modified:   src/test/scala/chiselTests/AnnotatingDiamondSpec.scala
	both modified:   src/test/scala/chiselTests/experimental/hierarchy/Annotations.scala
	both modified:   src/test/scala/circtTests/stage/ChiselStageSpec.scala

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@mergify mergify bot added Backport Automated backport, please consider for minor release bp-conflict labels Feb 14, 2025
@github-actions github-actions bot added the Deprecation Deprecates an API, will be included in release notes label Feb 14, 2025
@chiselbot chiselbot merged commit 8918516 into 6.x Feb 15, 2025
15 checks passed
@chiselbot chiselbot deleted the mergify/bp/6.x/pr-4643 branch February 15, 2025 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport Automated backport, please consider for minor release Deprecation Deprecates an API, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants