SynchronizerPrimitiveShiftReg: fix Dedup Issue #2547
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
mwachs5
commented
Jul 2, 2020
mwachs5
force-pushed
the
synchonizer-reg-dedup
branch
from
e4268bb
to
38e3299
Compare
mwachs5
commented
Jul 8, 2020
mwachs5
commented
Jul 8, 2020
|
@Mergifyio update |
|
Command
|
…wo identical modules Update src/main/scala/util/SynchronizerReg.scala Update src/main/scala/util/SynchronizerReg.scala
mwachs5
force-pushed
the
synchonizer-reg-dedup
branch
from
630cf09
to
9b24d36
Compare
jackkoenig
reviewed
Nov 5, 2020
| @@ -100,13 +107,17 @@ object AsyncResetSynchronizerShiftReg { | |||
| apply (in, sync, init.litValue.toInt, None) | |||
| } | |||
|
|
|||
| // Note: This module may end up with a non-Bool type reset. | |||
| // But the Primitives within will always have Bool reset type. | |||
| @deprecated("SyncResetSynchronizerShiftReg is unecessary with Chisel3 inferred resets. Use ResetSynchronizerShiftReg which will use the inferred reset type.", "rocket-chip 1.2") | |||
There was a problem hiding this comment.
Since you now have to set the reset type, is this deprecation message correct?
hcook
approved these changes
Nov 5, 2020
jackkoenig
approved these changes
Nov 5, 2020
mwachs5
commented
Nov 5, 2020
Comment on lines
+110
to
112
| // Note: This module may end up with a non-Bool type reset. | ||
| // But the Primitives within will always have Bool reset type. | ||
| @deprecated("SyncResetSynchronizerShiftReg is unecessary with Chisel3 inferred resets. Use ResetSynchronizerShiftReg which will use the inferred reset type.", "rocket-chip 1.2") |
There was a problem hiding this comment.
Suggested change
| // Note: This module may end up with a non-Bool type reset. | |
| // But the Primitives within will always have Bool reset type. | |
| @deprecated("SyncResetSynchronizerShiftReg is unecessary with Chisel3 inferred resets. Use ResetSynchronizerShiftReg which will use the inferred reset type.", "rocket-chip 1.2") | |
| // Note: This module may end up with a non-Bool type reset. | |
| // But the Primitives within will always have Bool reset type. | |
| @deprecated("SyncResetSynchronizerShiftReg usage is discouraged. Use ResetSynchronizerShiftReg which will use the inferred reset type. Cast the reset to Bool if you truly want to enforce SyncResetSynchronizerShiftReg functionality.", "rocket-chip 1.2") |
Hmm, good point. What do you think of this instead (at least I should learn how to spell unnecessary)?
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.
It was intended that the SynchronizerPrimitiveShiftReg be identical, as they have suggestName applied that was intended to capture all variance in terms of functionality.
However, because they had inferred reset type (the reset is cast appropriately inside the Primitive module), the FIRRTL would not be identical at the point of Dedup and you could end up with 2 versions. The final verilog (since verilog dosen't distinguish between reset types) would be identical.
Since the Primitives are intended to be easy to replace with library-specific macro sells for synchronization, this is undesirable.
This change pushes the reset type up into the SynchronizerShiftReg (which still may end up with multiple copies) to make sure you only get one SynchronizerPrimitiveShiftReg of each type.
Related issue: None
Type of change: bug report
Impact: API modification
Development Phase: implementation
Release Notes
Correct the dedup behavior for the *ResetSynchronizerPrimitiveShiftReg so that you only end up with one copy.