[core] Deprecate BlackBox in favor of ExtModule #5103
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seldridge
added
the
Deprecation
seldridge
force-pushed
the
dev/seldridge/deprecate-extmodule-traits-2
branch
from
e783564 to
969a052
Compare
seldridge
force-pushed
the
dev/seldridge/deprecate-blackbox
branch
4 times, most recently
from
646af55 to
ee05439
Compare
seldridge
force-pushed
the
dev/seldridge/deprecate-blackbox
branch
from
ee05439 to
c24d0f1
Compare
jackkoenig
approved these changes
Dec 2, 2025
Contributor
jackkoenig
left a comment
jackkoenig
left a comment
There was a problem hiding this comment.
Looks like you did a good mix of keeping some BlackBoxes around (since we should still test the basic features during the deprecation) and updating others.
Base automatically changed from
dev/seldridge/deprecate-extmodule-traits-2
to
main
December 2, 2025 02:47
Member
Author
Yeah, that was the needle I was trying to thread here. I wanted to keep tests around that were specifically testing blackboxes, migrate missing tests to extmodules, and also have everything ready to just mass delete when we are gearing up for Chisel 8. |
Deprecate BlackBox and tell users to switch to ExtModule. BlackBox has a ton of baggage associated with it, due to it mandating, at runtime, a single `val io` call and no additional calls to `IO` while also magically removing the `io_` prefix in the generated FIRRTL. `ExtModule` is far cleaner and works as a user would expect. You can call `IO` as much as you want. No magic prefixing happens. This is slightly unfortunate as one could argue that `BlackBox` is a good name. However, you can also argue (more strongly I would add) that this is an "external module" and it would be better to call it as such. If this is controversial, we can consider a migration back from `ExtModule` to `BlackBox`. Migrate some tests that were only testing blackboxes to be duplicated in the external module tests. This will allow us to just delete this test later. This has three motivations: 1. `BlackBox` has always been janky for the reasons above and `ExtModule` is the saner generator API. 2. It's bad to have two similar, though subtly different ways of doing things. 3. The singular IO that `BlackBox` forces users to go through means that `BlackBox` cannot have domain information attached to it. This essentially means that `BlackBox` will be essentially unusable until domains gain support for being in subfields. It's easier to just deprecate `BlackBox` than to require that domains gain this complicated feature addition just now. Signed-off-by: Schuyler Eldridge <[email protected]>
seldridge
force-pushed
the
dev/seldridge/deprecate-blackbox
branch
from
c24d0f1 to
2718b82
Compare
Contributor
|
😌 |
Member
Author
|
@oharboe wrote:
I was actually extremely worried about community push back on this one... However, it seems there's general acceptance. I'm similarly . |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Deprecate BlackBox and tell users to switch to ExtModule. BlackBox has a
ton of baggage associated with it, due to it mandating, at runtime, a
single
val iocall and no additional calls toIOwhile also magicallyremoving the
io_prefix in the generated FIRRTL.ExtModuleis farcleaner and works as a user would expect. You can call
IOas much asyou want. No magic prefixing happens.
This is slightly unfortunate as one could argue that
BlackBoxis a goodname. However, you can also argue (more strongly I would add) that this
is an "external module" and it would be better to call it as such. If
this is controversial, we can consider a migration back from
ExtModuleto
BlackBox.Migrate some tests that were only testing blackboxes to be duplicated in
the external module tests. This will allow us to just delete this test
later.
This has three motivations:
BlackBoxhas always been janky for the reasons above andExtModuleis the saner generator API.
It's bad to have two similar, though subtly different ways of doing
things.
The singular IO that
BlackBoxforces users to go through means thatBlackBoxcannot have domain information attached to it. Thisessentially means that
BlackBoxwill be essentially unusable untildomains gain support for being in subfields. It's easier to just
deprecate
BlackBoxthan to require that domains gain this complicatedfeature addition just now.
This is stacked on #5102.
Release Notes
Deprecate BlackBox in favor of ExtModule.