Make isView safe outside of Builder context #4228
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
jackkoenig
commented
Jun 28, 2024
| @@ -273,7 +273,7 @@ package object dataview { | |||
| } | |||
|
|
|||
| // Safe for all Data | |||
| private[chisel3] def isView(d: Data): Boolean = d._parent.contains(ViewParent) | |||
| private[chisel3] def isView(d: Data): Boolean = d._parent.exists(_ == ViewParent) | |||
There was a problem hiding this comment.
This looks equivalent but because contains takes its argument by value, even if _parent is None, ViewParent must be initialized (it's a lazy val) which requires a Chisel context (because it's a Module)... yay side effects 🤮.
mwachs5
reviewed
Jun 28, 2024
| @@ -275,6 +275,11 @@ trait ShiftRightWidthBehavior { self: ChiselRunners => | |||
|
|
|||
| class UIntOpsSpec extends ChiselPropSpec with Matchers with Utils with ShiftRightWidthBehavior { | |||
|
|
|||
| // This is intentionally a val outside of any ScalaTest constructs to check that it is legal | |||
| // to create a literal outside of a Chisel context and *before* and Chisel contexts have been created | |||
mwachs5
approved these changes
Jun 28, 2024
jackkoenig
commented
Jun 28, 2024
mergify bot
pushed a commit
that referenced
this pull request
Jun 28, 2024
(cherry picked from commit 219a237)
14 tasks
mergify bot
pushed a commit
that referenced
this pull request
Jun 28, 2024
(cherry picked from commit 219a237)
14 tasks
mergify bot
pushed a commit
that referenced
this pull request
Jun 28, 2024
(cherry picked from commit 219a237)
14 tasks
chiselbot
pushed a commit
that referenced
this pull request
Jun 28, 2024
(cherry picked from commit 219a237) Co-authored-by: Jack Koenig <[email protected]>
chiselbot
pushed a commit
that referenced
this pull request
Jun 28, 2024
(cherry picked from commit 219a237) Co-authored-by: Jack Koenig <[email protected]>
chiselbot
pushed a commit
that referenced
this pull request
Jun 28, 2024
(cherry picked from commit 219a237) Co-authored-by: Jack Koenig <[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.
Contemplating views in naming (#4222) exposed a longstanding bug that I patched out recently in #4147. Basically
isViewdoes not work outside of Builder contexts. Now it is safe.We do have lots of tests for things like literals outside of Builder contexts, but they didn't trigger this because
ViewParentis alazy valand thus if any Chisel elaboration has occurred on the same thread, then the problem doesn't manifest. Thus this "test" looks very strange, but by virtue of defining a literal assigned to a val first thing in a ScalaTest spec, it ensures no Chisel elaboration has run before when this line executes, and the naming plugin + #4222 ensures that this value is checked if it's a view.Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
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.