Skip to content

Commit ab78548

Browse files
committed
Fix too restrictive optimization in Java RyuFloat impl.
When `e2 < 0`, current `RyuFloat` calculates `dvIsTrailingZeros` by the code below, which should be equivalent to `mv%2^(q-1) == 0`. ```java dvIsTrailingZeros = (q < FLOAT_MANTISSA_BITS) && (mv & ((1 << (q - 1)) - 1)) == 0; ``` https://github.com/ulfjack/ryu/blob/1264a946ba66eab320e927bfd2362e0c8580c42f/src/main/java/info/adams/ryu/RyuFloat.java#L207 If I understand correctly, `(q < FLOAT_MANTISSA_BITS)` is an optimization that works as follows: - The maximum value of `v` is `2^FLOAT_MANTISSA_BITS - 1` - Therefore, when `q` is greater than or equal to `FLOAT_MANTISSA_BITS`, `v` is never divisible by `2^q` - So, if `(q < FLOAT_MANTISSA_BITS)` is false, we can skip the `(mv & ((1 << (q - 1)) - 1)) == 0` check **However, I think it should be `(q - 1 < FLOAT_MANTISSA_BITS + 2)` because**: - We're checking if `mv` is divisible by `2^(q-1)`, not `2^q`. So the left-hand side should be `q - 1` - The maximum value of `mv` is larger than `2^FLOAT_MANTISSA_BITS - 1`: - `mv` is `4 * m`, where `m` is the 23-bit mantissa. This means `mv <= 4 * (2^23 - 1) = 2^25 - 4` - To check if `mv` is divisible by `2^(q-1)`, the condition `q < 23` is too restrictive, because `mv` can be divisible by up to `2^24` - Therefore, the right-hand side should be `FLOAT_MANTISSA_BITS + 2` --- Actually, a test from `f2s_test.cc`'s `lotsOfTrailingZeros` fails in the Java implementation: https://github.com/ulfjack/ryu/blob/1264a946ba66eab320e927bfd2362e0c8580c42f/ryu/tests/f2s_test.cc#L69 ``` 1) lotsOfTrailingZeros(info.adams.ryu.RyuFloatTest) org.junit.ComparisonFailure: expected:<0.002441406[2]> but was:<0.002441406[3]> ``` This is because: - `mv = 41943040`, `e2 = -34`, and `q = 23` - Therefore, `dvIsTrailingZeros = false` because `q < 23` is false (but it should be true) - By step 4, `dv` is reduced from `244140625` to `24414062` (with `lastRemovedDigit = 5`) - The condition `(dvIsTrailingZeros && (lastRemovedDigit == 5) && (dv % 2 == 0))` evaluates to false (but it should be true!), because `dvIsTrailingZeros = false` https://github.com/ulfjack/ryu/blob/1264a946ba66eab320e927bfd2362e0c8580c42f/src/main/java/info/adams/ryu/RyuFloat.java#L267-L269 As a result, it is rounded up to `24414063`, but it should be rounded to `24414062`.
1 parent 1264a94 commit ab78548

File tree

2 files changed

+9
-1
lines changed

2 files changed

+9
-1
lines changed

src/main/java/info/adams/ryu/RyuFloat.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ public static String floatToString(float value, RoundingMode roundingMode) {
204204
}
205205

206206
dpIsTrailingZeros = 1 >= q;
207-
dvIsTrailingZeros = (q < FLOAT_MANTISSA_BITS) && (mv & ((1 << (q - 1)) - 1)) == 0;
207+
dvIsTrailingZeros = (q < FLOAT_MANTISSA_BITS + 3) && (mv & ((1 << (q - 1)) - 1)) == 0;
208208
dmIsTrailingZeros = (mm % 2 == 1 ? 0 : 1) >= q;
209209
}
210210
if (DEBUG) {

src/test/java/info/adams/ryu/FloatToStringTest.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,14 @@ public void roundingModeEven() {
7676
assertF2sEquals("3.436672E10", "3.4366718E10", 3.4366717E10f);
7777
}
7878

79+
@Test
80+
public void lotsOfTrailingZeros() {
81+
assertF2sEquals("2.4414062E-4", 2.4414062E-4f);
82+
assertF2sEquals("0.0024414062", 2.4414062E-3f);
83+
assertF2sEquals("0.0043945312", 4.3945312E-3f);
84+
assertF2sEquals("0.0063476562", 6.3476562E-3f);
85+
}
86+
7987
@Test
8088
public void roundingEvenIfTied() {
8189
assertF2sEquals("0.33007812", 0.33007812f);

0 commit comments

Comments
 (0)