Skip to content

Conversation

seldridge
Copy link
Member

@seldridge seldridge commented Jul 31, 2024

Add a new annotation/option to Chisel, RemapLayer, which is intended to
be used to allow a Chisel compilation to change which layers are used by
an upstream project's libraries in a downstream project. The motivation
here is to fix mismatches between upstream and downstream when they have
different layers.

Add the ability for ChiselStage to globally remap a layer to another
layer. This is intended to be used as a Chisel-level mechanism for a
downstream project to change the layers of all upstream projects. This
may be necessary in order to produce compatible FIRRTL designs where the
upstream project and downstream project have different layers, but the
upstream project has put certain constructs (e.g., asserts) into
specific layers.

Release Notes

Add --remap-layers ChiselStage option for changing all layers in an upstream project.

Add a new annotation/option to Chisel, `RemapLayer`, which is intended to
be used to allow a Chisel compilation to change which layers are used by
an upstream project's libraries in a downstream project.  The motivation
here is to fix mismatches between upstream and downstream when they have
different layers.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Add the ability for ChiselStage to globally remap a layer to another
layer.  This is intended to be used as a Chisel-level mechanism for a
downstream project to change the layers of all upstream projects.  This
may be necessary in order to produce compatible FIRRTL designs where the
upstream project and downstream project have different layers, but the
upstream project has put certain constructs (e.g., asserts) into
specific layers.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge added the Feature New feature, will be included in release notes label Jul 31, 2024
@seldridge seldridge requested a review from jackkoenig July 31, 2024 04:57
@seldridge
Copy link
Member Author

This also needs to remap layer colored probes.

@seldridge seldridge marked this pull request as draft July 31, 2024 05:04
@seldridge
Copy link
Member Author

This should also probably grow a feature for remapping to the root layer with the effect. This has overlap with CIRCT specialization flows, but is necessary in order to produce legal FIRRTL.

@sequencer
Copy link
Member

Should this feature goes to circt by adding some cmdline option?

@sequencer
Copy link
Member

another idea is we can simply append the package name of each layer to identify them(or provide an user defined override-able val instead.)

@seldridge
Copy link
Member Author

Should this feature goes to circt by adding some cmdline option?

A variation of this will need to show up as a FIRRTL language feature. However, I don't want to have CIRCT handle corrections to illegal FIRRTL to make it legal. I want to have Chisel always be able to emit legal FIRRTL.

E.g., we need some kind of "layer alias" or "layer remapping" at an extmodule boundary:

circuit Foo:
  layer A:
  extmodule Bar:
    layer B as A:
  module Foo:
    inst bar of Bar    

In the above Bar was compiled to Verilog with a layer Bar::B. (Which means it has a bunch of optional functionality that can be enabled by including a file layers_Bar_B.sv in the compilation). From the FIRRTL side, we need to know how to enable layer B in relation to the existing layers that Foo has. The above would cause layers A and Bar::B to be enabled at the same time and allow for any intermixing of layer colors A and Bar::B`

another idea is we can simply append the package name of each layer to identify them(or provide an user defined override-able val instead.)

I wanted a more Chisel language level way of doing this but couldn't come up with something. The problem is that there is no "global configuration" for Chisel where you can do this. Where could you put a val that would set this globally? It could be an implicit that is passed everywhere. However, that implicit has the possibility of being overridden and for you to get legal FIRRTL out of Chisel it can't be overridden. There's similar problems with things like withLayer[A](thunk: => A) which can be overridden by some upstream library.

All that said, practically, I don't think that this feature will be frequently used. I want most people to align on the upstream provided layers that Chisel has. Draft PR here: https://github.com/chipsalliance/chisel/pull/4323/files#diff-ef2aee2c3e90ffd56af995e76ac6573da3db4f939ea66b5c2f95756c29041504 These then give us a common place to put assertions for standard library generators or internal things. Users are then free to add more layers based on what they care about. (There is a missing feature to add a layer that is a parent of an existing layer.)

@seldridge
Copy link
Member Author

This also needs to remap layer colored probes.

Added in: fdfdff9

@seldridge seldridge marked this pull request as ready for review July 31, 2024 16:46
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

Generally looks good, a couple of nits and a suggestion

Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

LGTM, but still see nits.

@jackkoenig jackkoenig added this to the 7.0 milestone Jul 31, 2024
@seldridge seldridge enabled auto-merge (squash) July 31, 2024 22:35
@seldridge seldridge merged commit b7fa4e4 into main Jul 31, 2024
13 checks passed
@seldridge seldridge deleted the dev/seldridge/add-remap-layers branch July 31, 2024 22:46
SpriteOvO pushed a commit that referenced this pull request Aug 6, 2024
Add a new annotation/option to Chisel, `RemapLayer`, which is intended to
be used to allow a Chisel compilation to change which layers are used by
an upstream project's libraries in a downstream project.  The motivation
here is to fix mismatches between upstream and downstream when they have
different layers.

Add the ability for ChiselStage to globally remap a layer to another
layer.  This is intended to be used as a Chisel-level mechanism for a
downstream project to change the layers of all upstream projects.  This
may be necessary in order to produce compatible FIRRTL designs where the
upstream project and downstream project have different layers, but the
upstream project has put certain constructs (e.g., asserts) into
specific layers.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants