Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preserve literals across .asTypeOf #4168

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Preserve literals across .asTypeOf #4168

merged 1 commit into from
Jul 3, 2024

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Jun 12, 2024 •
edited
Loading

Built on top of #4160 so should merge this one after that one (might need rebase but git seems smarter about cherry-picks these days).

This is very valuable for ChiselSim-style testing because it means that you can much more easily construct literals in tests.

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • API modification

Desired Merge Strategy

  • Squash

Release Notes

Casting a literal (of any type) to another type with .asTypeOf will result in a literal of the new type. For non-literals, the FIRRTL representation will now be a little bit more efficient.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@jackkoenig jackkoenig added the Feature New feature, will be included in release notes label Jun 12, 2024
@jackkoenig jackkoenig added this to the 6.x milestone Jun 12, 2024
@jackkoenig jackkoenig requested review from seldridge and mwachs5 June 12, 2024 03:51
@jackkoenig jackkoenig force-pushed the preserve-literals-asTypeOf branch from d285777 to 78a7e9a Compare June 12, 2024 04:11
@jackkoenig jackkoenig modified the milestones: 6.x, 7.0 Jun 12, 2024
@jackkoenig jackkoenig added API Modification and removed Feature New feature, will be included in release notes labels Jun 12, 2024
@jackkoenig
Copy link
Contributor Author

I was hoping to backport this because I think it's potentially a very useful feature for ChiselSim and 6.x is closer to 3.6.x which makes for easier porting, but alas it seems that it is a really common pattern to use 0.U.asTypeOf(new MyBundle) to initialize a Wire of type MyBundle that then as a few fields connected to.

This change is still good and worth making, but it needs to be on a major version since it will break those uses (they just need to add a WireInit).

@jackkoenig jackkoenig marked this pull request as draft June 13, 2024 04:57
@jackkoenig
Copy link
Contributor Author

Converted to draft temporarily. I want to deprecate assigning to the wire returned by .asTypeOf (and backport to 6.x) before merging this. I am almost done with the PR doing that deprecation (which involves adding my long desired .readOnly feature!), should open it tomorrow.

This was referenced Jun 13, 2024
Open
Merged
@mergify mergify bot mentioned this pull request Jun 20, 2024
Merged
14 tasks
@jackkoenig jackkoenig force-pushed the preserve-literals-asTypeOf branch from 78a7e9a to 88f93e7 Compare June 25, 2024 20:39
@jackkoenig jackkoenig marked this pull request as ready for review June 25, 2024 20:39
@jackkoenig
Copy link
Contributor Author

This is ready to go

@jackkoenig jackkoenig force-pushed the preserve-literals-asTypeOf branch from 88f93e7 to 982664d Compare June 25, 2024 22:11
jackkoenig
jackkoenig commented Jun 26, 2024
View reviewed changes
src/test/scala/chiselTests/stage/WarningConfigurationSpec.scala
Comment on lines -383 to 387
it("should number AsTypeOfReadOnly as 8") {
val args = Array("--warn-conf", "id=8:e,any:s", "--throw-on-first-error")
it("should now error on AsTypeOfReadOnly") {
val args = Array("--throw-on-first-error")
val e = the[Exception] thrownBy ChiselStage.emitCHIRRTL(new AsTypeOfReadOnly, args)
e.getMessage should include("[W008] Return values of asTypeOf will soon be read-only")
e.getMessage should include("Return values of asTypeOf are now read-only")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the first time we'll be elevating a warning to an error. I haven't really worked through it all, we could add a corresponding E008 for the old W008, but no other errors are numbered yet (and of course, unlike warnings, you will not be allowed to configure errors). Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like if we want to add error numbers we should do that more holistically? separately from this PR?

@jackkoenig jackkoenig force-pushed the preserve-literals-asTypeOf branch from 982664d to de6d98e Compare June 28, 2024 17:55
@jackkoenig jackkoenig force-pushed the preserve-literals-asTypeOf branch from de6d98e to ca5062c Compare July 2, 2024 21:29
@jackkoenig
Preserve literals across .asTypeOf
f487c92 
Casting a literal (of any type) to another type with .asTypeOf will
result in a literal of the new type. For non-literals, the FIRRTL
representation will now be a little bit more efficient.
@jackkoenig jackkoenig force-pushed the preserve-literals-asTypeOf branch from ca5062c to f487c92 Compare July 2, 2024 23:40
@jackkoenig jackkoenig merged commit 8f00067 into main Jul 3, 2024
15 checks passed
@jackkoenig jackkoenig deleted the preserve-literals-asTypeOf branch July 3, 2024 22:33
tymcauley added a commit to tymcauley/fixedpoint that referenced this pull request Jul 16, 2024
@tymcauley
wip: Update Chisel from 6.5.0 to 7.0.0
218579f 
`UnknownWidth` became a `case object` in this Chisel PR:
chipsalliance/chisel#4242

The `connectFromBits` method was removed in this Chisel PR:
chipsalliance/chisel#4168
@tymcauley tymcauley mentioned this pull request Jul 16, 2024
Draft
tymcauley added a commit to tymcauley/fixedpoint that referenced this pull request Jul 16, 2024
@tymcauley
wip: Update Chisel from 6.5.0 to 7.0.0
28b609f 
`UnknownWidth` became a `case object` in this Chisel PR:
chipsalliance/chisel#4242

The `connectFromBits` method was removed in this Chisel PR:
chipsalliance/chisel#4168
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwachs5 mwachs5 mwachs5 approved these changes

@seldridge seldridge Awaiting requested review from seldridge

Assignees
No one assigned
Labels
API Modification
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jackkoenig @mwachs5