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

remove DefRegInit, change DefReg API with option definition. #1944

Merged
merged 4 commits into from Aug 17, 2021
Merged

remove DefRegInit, change DefReg API with option definition. #1944

merged 4 commits into from Aug 17, 2021
+7 −9

Conversation

sequencer
Copy link
Member

@sequencer sequencer commented May 28, 2021 •
edited

I'm currently working on cleaning chisel ir, I found it is strange to map both chisel3.internal.firrtl.DefRegInit and chisel3.internal.firrtl.DefReg to firrtl.ir.DefRegister. This PR corrects this.

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 state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • code cleanup

API Impact

None

Backend Code Generation Impact

None

Desired Merge Strategy

f- Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

Release Notes

None

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.x, 3.5.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@sequencer sequencer requested a review from seldridge May 28, 2021 09:46
mwachs5
mwachs5 reviewed May 28, 2021
View reviewed changes
core/src/main/scala/chisel3/internal/firrtl/IR.scala Outdated
case class DefReg(sourceInfo: SourceInfo, id: Data, clock: Arg) extends Definition
case class DefRegInit(sourceInfo: SourceInfo, id: Data, clock: Arg, reset: Arg, init: Arg) extends Definition
case class DefReg(sourceInfo: SourceInfo, id: Data, clock: Arg, reset: Option[Arg], init: Option[Arg]) extends Definition {
assert((reset.isDefined && init.isDefined) || (reset.isEmpty && init.isEmpty))
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the user experience if this fails? whats an example error message that they will get?

Copy link
Member Author

Choose a reason for hiding this comment

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

User will never touch this code, since it's internal API. I'll add a error message if some-devs come into this.

Copy link
Member Author

@sequencer sequencer May 28, 2021 •
edited

Choose a reason for hiding this comment

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

error message added in commit 919e965

seldridge
seldridge reviewed Jun 1, 2021
View reviewed changes
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

This saves some line count by changing an internal API. That seems fine to me.

I'd prefer to use Option[(Arg, Arg)] to avoid the need for a possible assertion failure (which admittedly should never occur).

core/src/main/scala/chisel3/internal/firrtl/IR.scala Outdated Show resolved Hide resolved
@sequencer sequencer requested a review from jackkoenig June 2, 2021 05:47
@jackkoenig jackkoenig added this to the 3.5.0 milestone Jun 14, 2021
jackkoenig
jackkoenig requested changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

If I recall correctly, there was some issue with either chiseltest or chisel-testers. Can you check if they both compile with this change @sequencer? And if not, please file corresponding PR(s) on the broken repo(s).

sequencer added a commit to sequencer/chisel-testers that referenced this pull request Jul 11, 2021
@sequencer
update for chipsalliance/chisel#1944
3c26b4c
sequencer added a commit to ucb-bar/chiseltest that referenced this pull request Jul 11, 2021
@sequencer
update for chipsalliance/chisel#1944
03b1400
@sequencer
Copy link
Member Author

ask for review again by @jackkoenig

@jackkoenig
Copy link
Contributor

Related chisel-testers PR: freechipsproject/chisel-testers#318
Related chiseltest PR: ucb-bar/chiseltest#332 (closed because the modified code was deleted)

@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Aug 17, 2021
@jackkoenig jackkoenig changed the title remove DefRegInit, change DefReg API with option defination. remove DefRegInit, change DefReg API with option definition. Aug 17, 2021
@mergify mergify bot merged commit ed894c6 into master Aug 17, 2021
jackkoenig added a commit that referenced this pull request Aug 18, 2021
@jackkoenig
Revert "remove DefRegInit, change DefReg API with option definition. (#…
136c76b 
…1944)"

This reverts commit ed894c6.
@jackkoenig jackkoenig mentioned this pull request Aug 18, 2021
Merged
14 tasks
mergify bot pushed a commit that referenced this pull request Aug 18, 2021
@jackkoenig
Revert "remove DefRegInit, change DefReg API with option definition. (#…
7c8a032 
…1944)" (#2080)

This reverts commit ed894c6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seldridge seldridge seldridge left review comments

@mwachs5 mwachs5 mwachs5 left review comments

@jackkoenig jackkoenig jackkoenig approved these changes

Assignees
No one assigned
Labels
Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sequencer @jackkoenig @seldridge @mwachs5