Create method for marking all ports of a `Module` with `DontTouchAnnotation` #3866

debs-sifive · 2024-02-23T19:33:57Z

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

Feature (or new API)

Desired Merge Strategy

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

Release Notes

Create dontTouch.modulePorts method for marking all ports of a Module with DontTouchAnnotation.

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.

…touch

seldridge · 2024-02-23T19:44:47Z

Is this necessary now that Public exists?

jackkoenig

Generally LGTM, I don't love the name dontTouch.modulePorts but I also don't have a better idea so 🤷‍♀️

@seldridge @azidar do you have any opinions on the name? Also this API makes it such that it will dontTouch any ports added via BoringUtils even after the function is called (because the call to DataMirror.fullModulePorts is delayed

jackkoenig · 2024-02-23T19:50:02Z

core/src/main/scala/chisel3/dontTouch.scala

+    *
+    * @param module whose ports shouldl not get optimized away
+    */
+  def modulePorts(module: BaseModule): BaseModule = {


You need an implicit SourceInfo here for DataMirror.fullModulePorts, that's why compilation is failing (incremental compile often hides this particular problem, that you can't materialize source locators in chisel core).

jackkoenig · 2024-02-23T19:53:13Z

core/src/main/scala/chisel3/dontTouch.scala

+      def toFirrtl: Seq[Annotation] = (DataMirror.fullModulePorts(module).map {
+        case (_, data) => DontTouchAnnotation(data.toNamed)
+      })


fullModulePorts is returns every Aggregate and Element so this will result in dontTouching things multiple times. Can we instead maybe just use .modulePorts and call dontTouch.apply above (which does the recursion and only marks Elements)?

jackkoenig · 2024-02-23T19:55:35Z

Is this necessary now that Public exists?

I agree with moving away from dontTouch generally but Public does impose additional constraints w.r.t. inferred resets and port widths that dontTouch doesn't.

mwachs5 · 2024-02-23T20:00:35Z

core/src/main/scala/chisel3/dontTouch.scala


+  /** Mark a module so that its ports will not get optimized away.
+    *
+    * @param module whose ports shouldl not get optimized away


Suggested change

* @param module whose ports shouldl not get optimized away

* @param module whose ports should not get optimized away

mwachs5 · 2024-02-23T20:01:09Z

core/src/main/scala/chisel3/dontTouch.scala

    data
  }

+  /** Mark a module so that its ports will not get optimized away.


does this already do the right thing w.r.t. property ports and probe ports?

As we discussed in dev, we're fine to add this API but we should direct users to Public and Probe here in the API docs depending on the use case.

seldridge · 2024-02-23T20:15:59Z

I agree with moving away from dontTouch generally but Public does impose additional constraints w.r.t. inferred resets and port widths that dontTouch doesn't.

Is not enforcing those constraints a requirement?

I'm trying to conserve concepts here and if this API is being used to create testable units, then I think those also probably want public and what it entails.

I won't hold this up as don't touch is a thing and having a utility to don't touch all the ports is generally useful. However, this is an intermediate API that I think we generally want to move off of.

jackkoenig · 2024-02-23T20:42:54Z

I think there are 3 main historical use cases of this API:

Give me these module ports "as is" in the Verilog -- as you noted this is just Public
Give me the ports of this module such that I can access them with Verilog XMRs -- this can be handled with Probes and RWProbes
I don't understand what is happening and want to debug, leave my ports alone so I can see them in the Verilog while I debug! -- I don't think we really have a great way to do this. Public sort of, but now the additional constraints are a problem when all you're trying to do is debug. Probes, again sort of, but you have to thread them to the top (BoringUtils helps but still extra boilerplate), and then of course read Probes intentionally do not block optimizations and RWProbes also are not really talking about keeping the ports themselves as much as keeping a forceable reference around.

(1) and (2) are what you should do in code that actually hits main, but (3) is still important IMO.

seldridge · 2024-02-23T21:38:50Z

I think that makes sense. Is there anyway that we can prevent users using (3) in situations where they should be using (1) or (2)? Annoyingly, all I could think of is not providing this API. 🙃

mwachs5 · 2024-02-26T23:39:39Z

src/test/scala/chiselTests/DontTouchSpec.scala

  }
+
+  "dontTouch.modulePorts" should "mark all ports of a module" in {
+    class ModuleWithPorts extends Module {


can we add a test for a Module with property and/or Probe ports? Im unclear if those are expected to be annotated, or there is supposed to be an error, or...?

debs-sifive added 3 commits February 22, 2024 16:12

skeleton

51faac7

Merge branch 'main' of github.com:chipsalliance/chisel into debs/dont…

5cbdc84

…touch

create dontTouch.modulePorts and tests

bbda70f

debs-sifive added the Feature New feature, will be included in release notes label Feb 23, 2024

debs-sifive requested a review from jackkoenig February 23, 2024 19:33

jackkoenig reviewed Feb 23, 2024

View reviewed changes

mwachs5 reviewed Feb 23, 2024

View reviewed changes

mwachs5 reviewed Feb 26, 2024

View reviewed changes

	* @param module whose ports shouldl not get optimized away
	* @param module whose ports should not get optimized away

Create method for marking all ports of a Module with DontTouchAnnotation #3866

Are you sure you want to change the base?

Create method for marking all ports of a Module with DontTouchAnnotation #3866

Uh oh!

Conversation

debs-sifive commented Feb 23, 2024

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Uh oh!

seldridge commented Feb 23, 2024

Uh oh!

jackkoenig left a comment

Choose a reason for hiding this comment

Uh oh!

jackkoenig Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

jackkoenig Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

jackkoenig commented Feb 23, 2024

Uh oh!

mwachs5 Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

mwachs5 Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

jackkoenig Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

seldridge commented Feb 23, 2024

Uh oh!

jackkoenig commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seldridge commented Feb 23, 2024

Uh oh!

mwachs5 Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Create method for marking all ports of a `Module` with `DontTouchAnnotation` #3866

Create method for marking all ports of a `Module` with `DontTouchAnnotation` #3866

jackkoenig commented Feb 23, 2024 •

edited

Loading