Layers: color check all not any and don't consider enabled layers. #4434
Conversation
Drop test specifically checking this incorrect behavior. Add tests checking don't need to be under a layer block to define a probe with more colors, and that enabled layers don't change what is allowed to be defined.
| matchesAndOmits(ChiselStage.emitCHIRRTL(new Foo))( | ||
| "define x = probe(a)", | ||
| "define y = probe(b)" | ||
| )() |
There was a problem hiding this comment.
ChiselStage.elaborate is sufficient to only check that it doesn't error.
Ditto below.
| val b = Wire(Bool()) | ||
| define(a, ProbeValue(b)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Great to remove this (and fix the checking) given that this emits with an illegal reference. This could be made to work, but we don't even have a way to describe this in the FIRRTL spec, i.e., there is no layerblock A.C.
(You know all this! I'm more leaving comments for bread crumbs if others come back to this later...)
There was a problem hiding this comment.
Breadcrumbs are great! Thanks! I'm unsure about this one, let's check!
I think the code in the test should not be supported as the probe target is not guaranteed to exist when A is enabled (C must be enabled, so the probe must at least have that color).
Additionally, enabling a layer should still allow sending out probes that aren't colored (not required to have color A [1].
This matches CIRCT's interpretation, AFAIK (FWIW).
I'm not sure what the mentioned A.C would be, and if it's some sort of sets-of-layers specification (?) not sure that changes anything here?
| @@ -180,8 +180,7 @@ package object internal { | |||
| color | |||
| case _ => return | |||
| } | |||
| val enabledLayers = Builder.enabledLayers.view ++ Builder.layerStack.headOption | |||
| if (enabledLayers.exists(_.canWriteTo(destLayer))) { | |||
| if (Builder.layerStack.headOption.forall(_.canWriteTo(destLayer))) { | |||
Drop test specifically checking this incorrect behavior.
Add tests checking don't need to be under a layer block to define a probe with more colors, and that enabled layers don't change what is allowed to be defined.
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Fix checking of layer-color probes to avoid rejecting valid uses and to detect actual problems that would otherwise be caught by CIRCT.
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.