Properly invalidate submodule IOs in tests #745
Conversation
| @@ -32,6 +32,7 @@ class PipeInternalWires extends Module { | |||
|
|
|||
| class CrossConnectTester(inType: Data, outType: Data) extends BasicTester { | |||
| val dut = Module(new CrossConnects(inType, outType)) | |||
| dut.io := DontCare | |||
There was a problem hiding this comment.
This seems odd: CrossConnects.io is bidirectional, I'm surprised this doesn't error (or does it?) out when assigning DontCare to it.
There was a problem hiding this comment.
Yeah it works fine. Awkwardly just connecting to the actual input port errors because it's a Clock and apparently connecting DontCare to Clocks directly is an error
There was a problem hiding this comment.
Wait what is this code doing then? DontCare shouldn't have any effect on outputs, and the bundle connect should recurse to the inputs. Any thoughts why the behavior differs? That also sounds like a bug...
There was a problem hiding this comment.
It does recurse and mark both as invalid including the output. Firrtl allows this, it's just ignored.
I think it is sensible to mark bidirectional aggregates DontCare. IMO it's a useful shorthand rather than having to call out each submodule input or your own module output. We should fix the mismatch between marking a clock DontCare yet still being able to just mark it's parent aggregate DontCare.
Let's discuss these issues before 3.1!
| @@ -51,7 +51,8 @@ class BrokenVectorPacketModule extends Module { | |||
| } | |||
|
|
|||
| class VectorPacketIOUnitTester extends BasicTester { | |||
| val device_under_test = Module(new BrokenVectorPacketModule) | |||
| val dut = Module(new BrokenVectorPacketModule) | |||
| dut.io <> DontCare | |||
There was a problem hiding this comment.
These tests don't check circuit behavior at all? They just elaborate? (in which case, another PR might be just to have them elaborate instead of run)
There was a problem hiding this comment.
It looks like it's a test for an old bug in Firrtl, what it really needs is "compileToVerilog".
Anyway this PR was just intended to get my very important Firrtl bugfix to work. It's a pretty scary bug that drops logic on connections to submodule ports. It must be in 3.0.1/1.01
| @@ -32,6 +32,7 @@ class PipeInternalWires extends Module { | |||
|
|
|||
| class CrossConnectTester(inType: Data, outType: Data) extends BasicTester { | |||
| val dut = Module(new CrossConnects(inType, outType)) | |||
| dut.io := DontCare | |||
There was a problem hiding this comment.
Wait what is this code doing then? DontCare shouldn't have any effect on outputs, and the bundle connect should recurse to the inputs. Any thoughts why the behavior differs? That also sounds like a bug...
Needed for chipsalliance/firrtl#706
Firrtl wasn't properly checking that submodule ports were invalidated, now it will. The Firrtl PR is failing the Chisel tests without this fix.
Type of change
Impact
Development Phase
Release Notes
Fix tests