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

toBigInt should round identically for Double and BigDecimal #3921

Merged
merged 3 commits into from
Mar 12, 2024
Merged

toBigInt should round identically for Double and BigDecimal #3921

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dac52bd
toBigInt should round identically for Double and BigDecimal
mvanbeirendonck Mar 8, 2024
cf10496
Avoid wildcard RoundingMode import
mvanbeirendonck Mar 11, 2024
62c81fb
Use overloading instead of default arguments
jackkoenig Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions core/src/main/scala/chisel3/Num.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package chisel3
import chisel3.internal.sourceinfo.SourceInfoTransform

import scala.language.experimental.macros
import scala.math.BigDecimal.RoundingMode._
mvanbeirendonck marked this conversation as resolved.
Show resolved Hide resolved
import chisel3.experimental.SourceInfo

// REVIEW TODO: Further discussion needed on what Num actually is.
Expand Down Expand Up @@ -205,9 +206,9 @@ trait NumObject {
* @param binaryPoint a binaryPoint that you would like to use
* @return
*/
def toBigInt(x: BigDecimal, binaryPoint: Int): BigInt = {
def toBigInt(x: BigDecimal, binaryPoint: Int, roundingMode: RoundingMode = HALF_UP): BigInt = {
Copy link
Contributor

@jackkoenig jackkoenig Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since this is a bugfix, I'd like to backport it. Stable branches check binary compatibility and obnoxiously in Scala (2 at least, not sure about 3) it is not binary compatible to add an argument with a default parameter--instead you have to just overload the method, i.e.:

def toBigInt(x: BigDecimal): BigInt = toBigInt(x, HALF_UP)
def toBigInt(x: BigDecimal, binaryPoint: Int, roundingMode: RoundingMode): BigInt = ...

I will push this change to your branch, I just wanted to note it before we merge.

Copy link
Contributor

@jackkoenig jackkoenig Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

~I just realized the version without the default argument already exists, I'm surprised this isn't causing a problem... ~, nevermind, that's the version taking Double, got it.

val multiplier = math.pow(2, binaryPoint)
val result = (x * multiplier).rounded.toBigInt
val result = (x * multiplier).setScale(0, roundingMode).toBigInt
result
}

Expand Down
14 changes: 14 additions & 0 deletions src/test/scala/chiselTests/LiteralExtractorSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,20 @@ class LiteralExtractorSpec extends ChiselFlatSpec {
bigIntFromDouble should be(bigInt53)
}

"doubles and big decimals" should "be rounded identically" in {

for (double <- Seq(1.0, 1.1, 1.5, 1.6, 2.5)) {
Copy link
Member

Choose a reason for hiding this comment

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

This has both 1.5 and 2.5 which means that this is checking to make sure it's not using HALF_EVEN. 👍


val bigDecimal = BigDecimal(double)

val bigIntFromDouble = Num.toBigInt(double, 0)
val bigIntFromBigDecimal = Num.toBigInt(bigDecimal, 0)

bigIntFromDouble should be(bigIntFromBigDecimal)
}

}

"literals declared outside a builder context" should "compare with those inside builder context" in {
class InsideBundle extends Bundle {
val x = SInt(8.W)
Expand Down
Loading