Relax legality of defines, align with FIRRTL spec #3857
Merged
Conversation
This file contains 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
Change the checking logic for the legality of a probe define destination to align with FIRRTL specification language introduced recently [1]. Specifically, this enforces the following sentence: > An operation that "writes" to a probe must be in a region whose layer > color is enabled when the probe's layer color is enabled. Previously, Chisel was more restrictive and would only allow writing to a probe whose color was the same as the layer block you are currently in. However, as has been worked out in CIRCT and elsewhere, it is fine to drive a probe from any region that is enabled when the probe is also enabled. Practically, this gets some Chisel code passing that was legal by a FIRRTL compiler, but Chisel was refusing to generate it. CC: @rwy7 [1]: chipsalliance/firrtl-spec@673aa08 Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
seldridge
commented
Feb 23, 2024
| layer.block(A.B) { | ||
| define(b, ProbeValue(in)) | ||
| define(y, ProbeValue(e)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This will produce illegal FIRRTL as the probes are defined more than once. However, it makes the test terser to write it this way.
jackkoenig
approved these changes
Feb 23, 2024
| val a = Wire(Bool()) | ||
| // Without this enable, this circuit is illegal because `C` is NOT enabled | ||
| // when `A` is enabled. Cf. tests checking errors of this later. | ||
| layer.enable(A) |
There was a problem hiding this comment.
Is enable the global enable for this Module?
There was a problem hiding this comment.
Yes, it's indicating that this module has layer A enabled. This will emit:
public module Foo enablelayer A :
; ...
If there is more than one enable, then those will get appended, e.g., layer.enable(A); layer.enable(B) will be:
public module Foo enablelayer A enablelayer B :
; ...
sequencer
pushed a commit
that referenced
this pull request
Feb 28, 2024
Change the checking logic for the legality of a probe define destination to align with FIRRTL specification language introduced recently [1]. Specifically, this enforces the following sentence: > An operation that "writes" to a probe must be in a region whose layer > color is enabled when the probe's layer color is enabled. Previously, Chisel was more restrictive and would only allow writing to a probe whose color was the same as the layer block you are currently in. However, as has been worked out in CIRCT and elsewhere, it is fine to drive a probe from any region that is enabled when the probe is also enabled. Practically, this gets some Chisel code passing that was legal by a FIRRTL compiler, but Chisel was refusing to generate it. CC: @rwy7 [1]: chipsalliance/firrtl-spec@673aa08 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
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.
Change the checking logic for the legality of a probe define destination to align with FIRRTL specification language introduced recently 1. Specifically, this enforces the following sentence:
Previously, Chisel was more restrictive and would only allow writing to a probe whose color was the same as the layer block you are currently in. However, as has been worked out in CIRCT and elsewhere, it is fine to drive a probe from any region that is enabled when the probe is also enabled.
Practically, this gets some Chisel code passing that was legal by a FIRRTL compiler, but Chisel was refusing to generate it.
CC: @rwy7
Release Notes
Align Chisel's checks of define destinations with the FIRRTL specification. Previously, Chisel was too strict.