Fix optimization of register with reset but invalid connection #2520
Conversation
Fixes #2516 Previously, reg r : UInt<8>, clock with : reset => (p, UInt<8>(3)) r is invalid would compile to: reg r : UInt<8>, clock r <= UInt<8>(0) now it compiles to: reg r : UInt<8>, clock r <= UInt<8>(3)
|
Needs some |
There was a problem hiding this comment.
(I spoke to Jack about this separately.)
I don't think the additional invalid interpretation here is correct or self-consistent with existing SFC behavior. The end-to-end compile of this should result in a mux with zero, not optimization to the reset value. My reasoning is that this pass is narrowly responsible for moving the reset mux in front of the register. Any further optimization should not happen and is introducing yet-another-interpretation of invalid that has to be special cased.
Put differently, this creates a reset mux not a reset when. I would expect the pass to generate or be equivalent to the following:
reg r : UInt<8>, clock
wire inv: UInt<8>
inv is invalid
r <= mux(p, UInt<8>(3), inv)reg r : UInt<8>, clock with : reset => (p, UInt<8>(3)) r is invalid now compiles to: reg r : UInt<8>, clock r <= mux(reset, UInt<8>(3), UInt<8>(0)) This is consistent with the behavior for a reset with an asynchronous reset.
|
Changed it to what @seldridge said. The implementation is a bit uglier, but it is consistent with the behavior when the register has an async reset. |
There was a problem hiding this comment.
LGTM
Weakening my previous statements: I would like to revisit the mux(cond, _, invalid) canonicalization in the future. It's annoying that this is not unified with when or validif formulations. Keeping clean separation of lowering (this pass) from optimization/folding will help us better tackle this fully in the future.
Fixes #2516 Previously, reg r : UInt<8>, clock with : reset => (p, UInt<8>(3)) r is invalid would compile to: reg r : UInt<8>, clock r <= UInt<8>(0) now it compiles to: reg r : UInt<8>, clock wire r_1 : UInt<8> r_1 is invalid r <= mux(reset, UInt<8>(3), r_1) This is consistent with the behavior for a reset with an asynchronous reset. (cherry picked from commit 5093da0) # Conflicts: # src/main/scala/firrtl/transforms/RemoveReset.scala # src/test/scala/firrtlTests/transforms/RemoveResetSpec.scala
Fixes #2516 Previously, reg r : UInt<8>, clock with : reset => (p, UInt<8>(3)) r is invalid would compile to: reg r : UInt<8>, clock r <= UInt<8>(0) now it compiles to: reg r : UInt<8>, clock wire r_1 : UInt<8> r_1 is invalid r <= mux(reset, UInt<8>(3), r_1) This is consistent with the behavior for a reset with an asynchronous reset. (cherry picked from commit 5093da0)
Fixes #2516 Previously, reg r : UInt<8>, clock with : reset => (p, UInt<8>(3)) r is invalid would compile to: reg r : UInt<8>, clock r <= UInt<8>(0) now it compiles to: reg r : UInt<8>, clock wire r_1 : UInt<8> r_1 is invalid r <= mux(reset, UInt<8>(3), r_1) This is consistent with the behavior for a reset with an asynchronous reset. (cherry picked from commit 5093da0)
mergify
bot
added
the
Backported
#2523) Fixes #2516 Previously, reg r : UInt<8>, clock with : reset => (p, UInt<8>(3)) r is invalid would compile to: reg r : UInt<8>, clock r <= UInt<8>(0) now it compiles to: reg r : UInt<8>, clock wire r_1 : UInt<8> r_1 is invalid r <= mux(reset, UInt<8>(3), r_1) This is consistent with the behavior for a reset with an asynchronous reset. (cherry picked from commit 5093da0) Co-authored-by: Jack Koenig <koenig@sifive.com>
#2522) Fixes #2516 Previously, reg r : UInt<8>, clock with : reset => (p, UInt<8>(3)) r is invalid would compile to: reg r : UInt<8>, clock r <= UInt<8>(0) now it compiles to: reg r : UInt<8>, clock wire r_1 : UInt<8> r_1 is invalid r <= mux(reset, UInt<8>(3), r_1) This is consistent with the behavior for a reset with an asynchronous reset. (cherry picked from commit 5093da0) Co-authored-by: Jack Koenig <koenig@sifive.com>
Fixes #2516
Previously,
would compile to:
now it compiles to:
Contributor Checklist
Type of Improvement
API Impact
This is a
Backend Code Generation Impact
No impact
Desired Merge Strategy
Release Notes
Fix lowering of synchronously reset registers that have an invalid connection.
Reviewer Checklist (only modified by reviewer)
Please Merge?