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

Update reported width from div/rem to match FIRRTL results #1748

Merged
merged 3 commits into from
Feb 1, 2021
Merged

Update reported width from div/rem to match FIRRTL results #1748

merged 3 commits into from
Feb 1, 2021

Conversation

albert-magyar
Copy link
Contributor

@albert-magyar albert-magyar commented Jan 21, 2021 •
edited
Loading

See #1746. I think it's reasonable to add a test where op result .getWidth values are compared against FIRRTL IR values using a few nested for loops, so I will update this soon.

Type of improvement: bug fix
API impact: getWidth now returns correct width for div/rem results
Backend code-generation impact: the most notable case is where a user supplies a computed width depending on getWidth as a specified width for a declared Wire/Reg/IO. These widths will change; while the error in the rem cases led to over-provisioned widths, the SInt div case led to excessively small widths. Therefore, the existing behavior of signed division in Chisel could have led to observable functional bugs in hardware.

Release Notes:
Previously, Chisel returned incorrect values when calling getWidth on the direct result of a % operation or signed division operation, yielding the width of the numerator rather than the actual result width. While the operation is computed with appropriate FIRRTL-defined widths, the results of getWidth can be used during hardware elaboration. The most notable case is where a user supplies a computed width depending on getWidth as a specified width for a declared Wire/Reg/IO. These widths will change; while the error in the rem cases led to over-provisioned widths, the SInt div case led to excessively small widths. Therefore, the previous behavior of signed division in Chisel could have led to observable functional bugs in hardware.

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?

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.0, 3.5.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@albert-magyar albert-magyar linked an issue Jan 21, 2021 that may be closed by this pull request
Disagreement between UInt.%, SInt.%, and SInt./ and corresponding Firrtl operations #1746
Closed
@aswaterman
Copy link
Member

@albert-magyar Thanks, and I agree your proposed tests would be beneficial in general.

@jackkoenig
Copy link
Contributor

This looks like a "backport all the way to 3.2.x" fix yeah?

@albert-magyar
Copy link
Contributor Author

This looks like a "backport all the way to 3.2.x" fix yeah?

Yes.

chick
chick approved these changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@chick chick left a comment

Choose a reason for hiding this comment

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

lgtm

@jackkoenig jackkoenig added this to the 3.2.x milestone Feb 1, 2021
@jackkoenig
Copy link
Contributor

I'm working on some tests, hope to push shortly then merge.

@jackkoenig
Copy link
Contributor

I added tests, they all fail without this change (except UInt division which was correct), and they all pass with this change.

@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Feb 1, 2021
@albert-magyar
Copy link
Contributor Author

Thanks for reminding me @jackkoenig, I had some tests that are better than nothing and test a broader space of widths/ops, but I hadn't pushed them since they felt a little silly. But test > no tests, so I added them, too.

@albert-magyar albert-magyar added the Bugfix Fixes a bug, will be included in release notes label Feb 1, 2021
@mergify mergify bot merged commit 98ce919 into master Feb 1, 2021
mergify bot pushed a commit that referenced this pull request Feb 1, 2021
@albert-magyar
Update reported width from div/rem to match FIRRTL results (#1748)
b0654ed 
* Update reported width from div/rem to match FIRRTL results

* Add tests for width of % and / on UInt and SInt

* Add loop-based test for known UInt/SInt op result widths

Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 98ce919)

# Conflicts:
#	core/src/main/scala/chisel3/internal/firrtl/IR.scala
@mergify mergify bot mentioned this pull request Feb 1, 2021
Closed
mergify bot pushed a commit that referenced this pull request Feb 1, 2021
@albert-magyar
Update reported width from div/rem to match FIRRTL results (#1748)
500a501 
* Update reported width from div/rem to match FIRRTL results

* Add tests for width of % and / on UInt and SInt

* Add loop-based test for known UInt/SInt op result widths

Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 98ce919)
@mergify mergify bot mentioned this pull request Feb 1, 2021
Merged
mergify bot pushed a commit that referenced this pull request Feb 1, 2021
@albert-magyar
Update reported width from div/rem to match FIRRTL results (#1748)
80f5569 
* Update reported width from div/rem to match FIRRTL results

* Add tests for width of % and / on UInt and SInt

* Add loop-based test for known UInt/SInt op result widths

Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 98ce919)
@mergify mergify bot mentioned this pull request Feb 1, 2021
Merged
@mergify mergify bot added the Backported This PR has been backported label Feb 1, 2021
mergify bot added a commit that referenced this pull request Feb 1, 2021
@mergify @albert-magyar
Update reported width from div/rem to match FIRRTL results (#1748) (#…
bbd62b2 
…1769)

* Update reported width from div/rem to match FIRRTL results

* Add tests for width of % and / on UInt and SInt

* Add loop-based test for known UInt/SInt op result widths

Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 98ce919)

Co-authored-by: Albert Magyar <albert.magyar@gmail.com>
@albert-magyar albert-magyar deleted the fix-div-rem-widths branch February 1, 2021 22:29
mergify bot added a commit that referenced this pull request Feb 1, 2021
@mergify @albert-magyar @jackkoenig
Update reported width from div/rem to match FIRRTL results (#1748) (#…
e419085 
…1770)

* Update reported width from div/rem to match FIRRTL results

* Add tests for width of % and / on UInt and SInt

* Add loop-based test for known UInt/SInt op result widths

Co-authored-by: Jack Koenig <koenig@sifive.com>
(cherry picked from commit 98ce919)

Co-authored-by: Albert Magyar <albert.magyar@gmail.com>
Co-authored-by: Jack Koenig <koenig@sifive.com>
@MAFOR-TECH MAFOR-TECH mentioned this pull request Oct 24, 2021
Closed
@sequencer sequencer mentioned this pull request Nov 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chick chick chick approved these changes

Assignees
No one assigned
Labels
Backported This PR has been backported Bugfix Fixes a bug, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Milestone
3.2.x
Development

Successfully merging this pull request may close these issues.

Disagreement between UInt.%, SInt.%, and SInt./ and corresponding Firrtl operations
4 participants
@albert-magyar @aswaterman @jackkoenig @chick