Stop Emitting BlackBoxResourceAnno

[seldridge]

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 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

Other.

API Impact

Change Chisel to never emit BlackBoxResourceAnno. This improves the separation between Chisel and a FIRRTL compiler. The semantics of locating the file associated with a BlackBoxResourceAnno is that of Java Resources. This is fine if a FIRRTL compiler is implemented on top of the JVM. However, if the FIRRTL compiler is not implemented on the JVM, then it makes the Chisel classpath part of what is necessary to pass to the FIRRTL compiler (and that a FIRRTL compiler needs to handle Java resource finding).

Backend Code Generation Impact

No.

Desired Merge Strategy

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

Release Notes

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.x, 3.5.0) ?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you mark as Please Merge?

[sequencer]

Here is a unrelated quetion: is possible to replace HasBlackBoxPath as well? Or even deprecate this two API with a type safe way to represent Path with os-lib. some thing like:
https://github.com/sinofp/firrtl/blob/e769d0e4861b4c17bd6c97e717e440b479e26120/src/main/scala/firrtl/FileUtils.scala#L188-L195

and a potential issue we might need to concern is: if there is a really huge blackbox verilog, something like 16 core Boom Verilog(maybe 1G+), will this become another performance issue to JVM? It will directly consume 1G from JVM, and case class has been copying this 1G in each transform.

Maybe another work around is that we can copy all blackboxes to targetDir at the end of elaboration(or reflect it to a chisel API, and do this during elaboration), finally set directly to the blackbox file to there.

[seldridge]

is possible to replace HasBlackBoxPath as well?

Yes. I was aiming to keep this PR narrowly focused, though.

Fundamentally, only one of the three blackbox annotations are actually needed. They all encode the same information with differences in how the path is encoded and at what time.

and a potential issue we might need to concern is: if there is a really huge blackbox verilog, something like 16 core Boom Verilog(maybe 1G+), will this become another performance issue to JVM? It will directly consume 1G from JVM, and case class has been copying this 1G in each transform.

Possibly. Using BlackBoxPathAnno may be a better way to encode this and could be a more sensible way to do what this PR does.

Maybe another work around is that we can copy all blackboxes to targetDir at the end of elaboration(or reflect it to a chisel API, and do this during elaboration), finally set directly to the blackbox file to there.

This could probably be done with some CustomFileEmission where the annotation produces a side effect. This would then happen at the end of the first stage (either Chisel or FIRRTL depending how its being run).

[sequencer]

This could probably be done with some CustomFileEmission where the annotation produces a side effect. This would then happen at the end of the first stage (either Chisel or FIRRTL depending how its being run).

“happen at the end of the first stage” sounds reasonable! if FIRRTL doesn’t need any modification in the later passes, I think we can free this memory as early as possible to save the effort of copying potential large text file.

[seldridge]

Converting these to a path has the downside of not being able to represent the full space of URLs that resources can resolve to (e.g., if this is getting dug out of some JAR file which may be package in a library). If we run into a problem with this requiring too much memory, I can look into an alternative solution.

[mwachs5]

is there anything in the website docs that should be updated for this change? https://www.chisel-lang.org/chisel3/docs/explanations/blackboxes.html#providing-implementations-for-blackboxes

[seldridge]

@mwachs wrote:

is there anything in the website docs that should be updated for this change?

No, I don't think so. 😄 What's cool about this is that the Chisel-level API doesn't change at all. Users can continue to use hasResource with no observable change except to the Annotation file that Chisel produces in a split Chisel/FIRRTL build.

[jackkoenig]

I think this is okay to backport, it could affect some advanced users doing stuff in FIRRTL. That being said, since it is technically equivalent, it shouldn't cause any problems.

[seldridge]

@chick, @jackkoenig: I tested this locally using https://github.com/seldridge/chisel-resource-test. This was initially not working. However, after cleaning my environment (again) it worked. This is pulling the JAR out of dsptools.

[chick]

This looks good to me. I like the tests. One quibble (which can be ignored regarding DRY code).
Only other consideration. Since we can anticipate that large files in a resource dir could cause problems whether
we could issue some warning based on size or copy time that would alert users to this situation. Just a thought

[seldridge]

@chick. Can you take one more look? Updated with your changes. To DRY it out, I used a implicit class on the object to get a BlackBoxInlineAnno.fromResource(...) API which I thought would be the most readable if someone inspects the addResource code.

[chick]

LGTM