Add Probes to .toString Data methods #4478
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
reviewed
Oct 17, 2024
jackkoenig
reviewed
Oct 17, 2024
Comment on lines
490
to
495
| case None => addProbeModifier(chiselType) | ||
| // Handle DontCares specially as they are "literal-like" but not actually literals | ||
| case Some(DontCareBinding()) => s"$chiselType(DontCare)" | ||
| case Some(topBinding) => | ||
| val binding: String = thiz._bindingToString(topBinding) | ||
| val name = thiz.earlyName |
There was a problem hiding this comment.
I'm sort of surprised the test is below is working:
Wire(probe.Probe(UInt(1.W))).toString should be("BoundDataModule.?: Wire[Probe<UInt<1>>]")A Wire of probe should have a top binding...
There was a problem hiding this comment.
It does have a top binding? Not sure I understand..
There was a problem hiding this comment.
Yes, it should have top binding, this does not make sense to me
There was a problem hiding this comment.
I think I missed that you also had addProbeModifier below.
I think it would be better to only do that once, will suggest above.
jackkoenig
reviewed
Oct 17, 2024
Comment on lines
477
to
484
| def addProbeModifier(chiselType: String): String = { | ||
| probeInfo match { | ||
| case None => chiselType | ||
| case Some(ProbeInfo(writeable, layer)) => | ||
| val layerString = layer.map(x => s"[${x.fullName}]").getOrElse("") | ||
| (if (writeable) "RWProbe" else "Probe") + s"$layerString<$chiselType>" | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggested change
| def addProbeModifier(chiselType: String): String = { | |
| probeInfo match { | |
| case None => chiselType | |
| case Some(ProbeInfo(writeable, layer)) => | |
| val layerString = layer.map(x => s"[${x.fullName}]").getOrElse("") | |
| (if (writeable) "RWProbe" else "Probe") + s"$layerString<$chiselType>" | |
| } | |
| } | |
| val chiselTypeWithModifier = | |
| probeInfo match { | |
| case None => chiselType | |
| case Some(ProbeInfo(writeable, layer)) => | |
| val layerString = layer.map(x => s"[${x.fullName}]").getOrElse("") | |
| (if (writeable) "RWProbe" else "Probe") + s"$layerString<$chiselType>" | |
| } |
Then use chiselTypeWithModifier below anywhere chiselType was being used.
jackkoenig
approved these changes
Oct 18, 2024
mergify bot
pushed a commit
that referenced
this pull request
Oct 18, 2024
* Add probe string to .toString Data methods * Update chisel tests with new probe toString (cherry picked from commit 8c718b2) # Conflicts: # core/src/main/scala/chisel3/Data.scala # src/test/scala/chisel3/TypeEquivalenceSpec.scala # src/test/scala/chiselTests/LayerSpec.scala # src/test/scala/chiselTests/ProbeSpec.scala
12 tasks
|
Thanks! |
jackkoenig
pushed a commit
that referenced
this pull request
Nov 26, 2024
* Add probe string to .toString Data methods * Update chisel tests with new probe toString (cherry picked from commit 8c718b2)
chiselbot
pushed a commit
that referenced
this pull request
Nov 26, 2024
* Add probe string to .toString Data methods * Update chisel tests with new probe toString (cherry picked from commit 8c718b2) Co-authored-by: Adam Izraelevitz <[email protected]>
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.
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Probe chisel types now include the kind of probe and layer in their
.toStringmethodReviewer 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.