Fix ActualDirection calculation from SpecifiedDirection #4205
Conversation
|
Converted to a draft to test on large designs internally as this may have unexpected effects. |
T1 passes. Waiting for SiFive's designs. |
|
No longer draft, this is ready to go. The breakages I found were due to #4218. |
|
I've switched this from 6.x to 7.0. This is a tough call but probably the right one. The main reason to backport is that #4198 exposes a subtle, rare bug hit in the T1 (#4198 (comment)). It's also just generally a bug in our direction calculation behavior. The reasons to not backport is that this is a change to a pretty core aspect of Chisel that easily could expose other bugs, whether in Chisel (which it does expose, see #4218), or in user code, especially if the user is using For that reason, I'm thinking we should save this for 7.0, even if it means that 6.5 may include rare breakages like the one experienced in T1. We can also try fixing that breakage in a much more focused way but I'm unsure how much effort we should put into it. |
| case SpecifiedDirection.Unspecified | SpecifiedDirection.Flip => ActualDirection.Unspecified | ||
| case SpecifiedDirection.Output => ActualDirection.Output | ||
| case SpecifiedDirection.Input => ActualDirection.Input | ||
| case SpecifiedDirection.Output | SpecifiedDirection.Unspecified => ActualDirection.Output |
There was a problem hiding this comment.
is this actually sound ...? Output is coercing all leaf members while Unspecified is not. I dont think this is equivalent
There was a problem hiding this comment.
It's unstated in the ScalaDoc but this function is only ever used to determine the ActualDirection for Elements. It wouldn't be correct for any Aggregate because it would need to handle ActualDirection.Bidirectional(_). As for coercion, chisel3.Output the operator coerces, but the ActualDirection it just reporting the final resolved direction for something after all flips and coercions from above have been applied.
| } | ||
| class MyModule extends RawModule { | ||
| val w = Wire(new MyBundle) | ||
| assert(DataMirror.specifiedDirectionOf(w) == SpecifiedDirection.Unspecified) |
There was a problem hiding this comment.
i agree with the tests tho
Unspecified direction maps to Output while Flip maps to Input. Previously, ActualDirection.fromSpecified would return Unspecified for either Unspecified or Flipped input. This in turn resulted in Bundles mixing Unspecified and Outputs as being "bidirectional" despite the fact that they are actually unidirectional (or passive). This change makes the direction calculation more consistent albeit at the cost of marking the directions of unspecified Data as Output.
sequencer
force-pushed
the
fix-directionality
branch
from
4f22aa2
to
352043f
Compare
|
@jackkoenig @mwachs5 is there any plan to get this PR upstreamed? |
- circt is not released yet for DPI, use main branch instead. - merge chipsalliance/chisel#4205 and chipsalliance/chisel#4205 in my chisel branch for T1.
- circt is not released yet for DPI, use main branch instead. - merge chipsalliance/chisel#4205 and chipsalliance/chisel#4205 in my chisel branch for T1.
- circt is not released yet for DPI, use main branch instead. - merge chipsalliance/chisel#4205 and chipsalliance/chisel#4205 in my chisel branch for T1.
| case SpecifiedDirection.Flip => ActualDirection.Bidirectional(ActualDirection.Flipped) | ||
| case _ => ActualDirection.Bidirectional(ActualDirection.Default) | ||
| } | ||
| throwException(s"Internal Error! Unhandled directionality of children: $childDirections for $this!") |
There was a problem hiding this comment.
Please add a test to cover this.
| val chirrtl = ChiselStage.emitCHIRRTL(new MyModule) | ||
| assert(chirrtl.contains("wire w : { flip foo : UInt<8>, flip bar : UInt<8>}")) | ||
| } | ||
|
|
There was a problem hiding this comment.
What is directionOf a wire that has input and output? Is that already tested in this file as bidirectional? Is that missing a test?
In #2634, we unified the Chisel 2 and Chisel 3 direction semantics, making it legal to have Records where the directionality of elements is mixed (some may be specified, some may be not). The implementation is mostly right, but there was a corner case I came across in #4204.
Previously, ActualDirection.fromSpecified would return Unspecified for either Unspecified or Flipped input. This in turn resulted in Bundles mixing Unspecified and Outputs as being "bidirectional" despite the fact that they are actually unidirectional (or passive), and similarly for Bundles mixing Flipped with Input.
Now, Unspecified maps to Output and Flip maps to Input. This change makes the direction calculation more consistent albeit at the cost of marking the directions of unspecified Data as Output.
I'm backporting this to 6.x because #4204 will manifest there, but this is a pretty big change to the reported direction of Chisel Data so I don't think we should backport it to other branches.
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Fixes #4204.
Unspecified direction maps to Output while Flip maps to Input. Previously, ActualDirection.fromSpecified would return Unspecified for either Unspecified or Flipped input. This in turn resulted in Bundles mixing Unspecified and Outputs as being "bidirectional" despite the fact that they are actually unidirectional (or passive).
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.