Add width utility functions to avoid incorrect usage of bare log2Ceil(). #819

ucbjrl · 2018-04-27T23:12:47Z

Related issue: #818

Type of change: other enhancement

Impact: API addition (no impact on existing code)

Development Phase: proposal

Release Notes
Add helper functions UnsignedBitsRequired and UnsignedWidthRequired to provide an alternative to the potentially incorrect usage of log2Ceil to calculate wire widths.

edwardcwang · 2019-03-22T19:41:04Z

Consensus from dev meeting: shorten names and general agreement to add something like this

chick

LGTM.
Nit Question:
Should your tests include the in: chisel3.Data signature?
E.g. UnsignedBitsRequired(8.U) or UnsignedBitsRequired(UInt(8.W))
Also: UnsignedBitsRequired(x) == UnsignedBitsRequired(x.W) for all x: Integer, right?

ucbjrl · 2019-05-24T20:08:28Z

Remove apply(Data) method
change name(s) to signedBitLength, unsignedBitLength

Remove apply(Data) method. Change name(s) to signedBitLength, unsignedBitLength.

ducky64 · 2019-06-18T00:23:13Z

src/main/scala/chisel3/util/Math.scala

+    require(in >= 0)
+    if (in == 0) {
+      // 0.bitLength returns 0 - thank you Java.
+      1


Was this the behavior we wanted? (I'm sure we brought this up, I don't remember the resolution and I don't think it was written down)

I believe so, but we can review at next dev meeting.

Return 0, not 1.

ducky64 · 2019-06-18T00:32:56Z

src/test/scala/chiselTests/Math.scala

+
+  property ("unsignedBitLength is computed correctly") {
+    forAll(safeUIntWidth) { case (width: Int) =>
+      for ( offset <- List(-1, 0, 1)) {


I'm not sure this test is testing anything since it's basically the duplicating the code used to implement unsignedBitLength. Consider just dropping in a few values and hand-coding the result.

Corrected in ec3e030

ducky64 · 2019-06-18T00:33:06Z

src/test/scala/chiselTests/Math.scala

+
+  property ("signedBitLength is computed correctly") {
+    forAll(safeUIntWidth) { case (width: Int) =>
+      for ( offset <- List(-1, 0, 1)) {


Corrected in ec3e030

src/main/scala/chisel3/util/Math.scala

Independently calculate the bit length to verify correct operation.

jackkoenig · 2019-07-01T18:45:27Z

@ducky64 thoughts on merging thisz?

jackkoenig · 2019-07-08T18:38:19Z

Ping @ducky64

ducky64 · 2019-07-08T18:39:24Z

Is the zero width fix implemented? It doesn't appear so?

ucbjrl · 2019-07-08T20:42:03Z

Thanks @ducky64 for pointing out the zero width fix hadn't yet been implemented. Should we be doing this for signed 0 as well? (Currently signedBitLength(0) == 1.)

ducky64 · 2019-07-10T23:34:56Z

The unsigned case looks fine. The signed zero case is a good question.

My thoughts for signed:
0 bits: 0
1 bit: 0, -1
2 bits: 1, 0, -1, -2
though more thoughts are welcome

I'm still not a fan of the test style since it's not simple enough to be obvious at a glance (eg, explicit testvectors) but also doesn't give you the better correctness guarantees from a more formal approach. In fact, the test code is much harder to reason about than the main code, because there's so much complexity going on. That being said, I don't care enough about this to block it. Once we resolve the signed zero, I'll give this a green.

ucbjrl · 2019-07-11T15:24:05Z

I agree with @ducky64, but I think there's enough confusion over this, that we shouldn't incorporate it. (I don't see how you can represent 0 without requiring any bits. Perhaps my lack of hardware knowledge is showing.)

jackkoenig · 2019-07-11T18:52:42Z

Zero-width wires are fine and we have rules for removing them.

The advantage of representing 0.U with zero bits is that you can perform operations on it without increasing the resulting width, eg.: wire +& 0.U(1.W) will have a resulting width 1 greater than that of wire, while wire +& 0.U(0.W) will have a width the same as that of wire. That being said, interval width inference could be an alternative mechanism for this feature and would work for literals of any width.

ucbjrl · 2019-07-11T19:36:59Z

But why add the constant 0.U in the first place? I would argue that this should be dealt with in optimizations for specific operations, not as a side-effect of the width required to store a specific constant. Similarly, x - 0.U, x + 0.S, x - 0.S, x * 1.U, and x * 1.S should be optimized away, regardless of the width of the 0 or 1 constant.
(and ditto for shifts by 0.U)

aswaterman · 2019-07-11T20:22:49Z

I’ll also add that zero-width zeros are particularly useful for Cat-like operations in generator code. Avoids special-casing the case that there’s only 1 of something, i.e., when log2(number of things) = 0.

…

On Thu, Jul 11, 2019 at 12:37 PM Jim Lawson ***@***.***> wrote: But why add the constant 0.U in the first place? I would argue that this should be dealt with in optimizations for specific operations, not as a side-effect of the width required to store a specific constant. Similarly, x - 0.U, x + 0.S, x - 0.S, x * 1.U, and x * 1.S should be optimized away. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#819>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAH3XQSGTIZH7XYS4ZI7YCLP66DV3ANCNFSM4E5MP4NQ> .

ucbjrl · 2019-07-11T20:39:46Z

Right. I keep forgetting the assumption that a zero-width wire has the value 0. I won't let my confusion hinder the adoption of this PR.

ducky64 · 2019-07-15T19:50:57Z

Resolution from today's meeting: signed 0 is 0 bits, signed -1 is 1 bit, signed 1 is 2 bits.

ducky64

looks fine.

Add width utility functions to avoid incorrect usage of bare log2Ceil().

476bcae

ucbjrl mentioned this pull request Apr 27, 2018

Chisel Standard Library #818

Open

Merge branch 'master' into bitsrequired

bf8e323

ucbjrl requested a review from a team as a code owner May 23, 2019 23:26

chick approved these changes May 24, 2019

View reviewed changes

ducky64 added this to the 3.2.0 milestone May 24, 2019

ucbjrl added 3 commits June 10, 2019 13:25

Merge branch 'master' into bitsrequired

d47e7ef

Respond to comments:

c401540

Remove apply(Data) method. Change name(s) to signedBitLength, unsignedBitLength.

Merge branch 'master' into bitsrequired

0175818

ucbjrl requested a review from chick June 17, 2019 17:24

ducky64 reviewed Jun 18, 2019

View reviewed changes

Respond to comments - don't be lazy.

ec3e030

Independently calculate the bit length to verify correct operation.

Merge branch 'master' into bitsrequired

a831502

Respond to comments - return in.bitLength - 0 (not 1) for 0

d67fa8c

Respond to comments - update wdith for signed 0; add explicit tests.

9171048

ucbjrl added 2 commits July 16, 2019 09:10

Merge branch 'master' into bitsrequired

fe9dbf1

Add comment expressing zero width wire assumption.

204cedb

ducky64 approved these changes Jul 17, 2019

View reviewed changes

ucbjrl merged commit ae784b6 into master Jul 18, 2019

ucbjrl deleted the bitsrequired branch July 18, 2019 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add width utility functions to avoid incorrect usage of bare log2Ceil(). #819

Add width utility functions to avoid incorrect usage of bare log2Ceil(). #819

ucbjrl commented Apr 27, 2018

edwardcwang commented Mar 22, 2019

chick left a comment

ucbjrl commented May 24, 2019

ducky64 Jun 18, 2019

ucbjrl Jun 18, 2019

ucbjrl Jun 24, 2019

ducky64 Jun 18, 2019

ucbjrl Jun 18, 2019

ducky64 Jun 18, 2019

ucbjrl Jun 18, 2019

jackkoenig commented Jul 1, 2019

jackkoenig commented Jul 8, 2019

ducky64 commented Jul 8, 2019

ucbjrl commented Jul 8, 2019

ducky64 commented Jul 10, 2019

ucbjrl commented Jul 11, 2019

jackkoenig commented Jul 11, 2019

ucbjrl commented Jul 11, 2019 •

edited

Loading

aswaterman commented Jul 11, 2019 via email

ucbjrl commented Jul 11, 2019

ducky64 commented Jul 15, 2019

ducky64 left a comment

Add width utility functions to avoid incorrect usage of bare log2Ceil(). #819

Add width utility functions to avoid incorrect usage of bare log2Ceil(). #819

Conversation

ucbjrl commented Apr 27, 2018

edwardcwang commented Mar 22, 2019

chick left a comment

Choose a reason for hiding this comment

ucbjrl commented May 24, 2019

ducky64 Jun 18, 2019

Choose a reason for hiding this comment

ucbjrl Jun 18, 2019

Choose a reason for hiding this comment

ucbjrl Jun 24, 2019

Choose a reason for hiding this comment

ducky64 Jun 18, 2019

Choose a reason for hiding this comment

ucbjrl Jun 18, 2019

Choose a reason for hiding this comment

ducky64 Jun 18, 2019

Choose a reason for hiding this comment

ucbjrl Jun 18, 2019

Choose a reason for hiding this comment

jackkoenig commented Jul 1, 2019

jackkoenig commented Jul 8, 2019

ducky64 commented Jul 8, 2019

ucbjrl commented Jul 8, 2019

ducky64 commented Jul 10, 2019

ucbjrl commented Jul 11, 2019

jackkoenig commented Jul 11, 2019

ucbjrl commented Jul 11, 2019 • edited Loading

aswaterman commented Jul 11, 2019 via email

ucbjrl commented Jul 11, 2019

ducky64 commented Jul 15, 2019

ducky64 left a comment

Choose a reason for hiding this comment

ucbjrl commented Jul 11, 2019 •

edited

Loading