Bug 185115 – Math.round() produces wrong result for value prior to 0.5

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

Math.round(0.499999999999999944) produces 1.0; it should produce 0.0. Likewise for the negated input.

Note 0.499999999999999944 is the largest representable double less than 0.5.

Created attachment 339079 [details]
Patch

Created attachment 339080 [details]
Patch

[Saam Barati]

Comment on attachment 339080 [details]
Patch

This doesn't address the bug in our JITs as well. Check out DFG/FTL ArithRound

[Ross Kirsling]

Created attachment 378145 [details]
Patch

[Ross Kirsling]

Comment on attachment 378145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378145&action=review

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1009
> +        jit.compareDouble(MacroAssembler::DoubleGreaterThan, SpecializedThunkJIT::fpRegT2, SpecializedThunkJIT::fpRegT0, SpecializedThunkJIT::regT0);
> +        jit.convertInt32ToDouble(SpecializedThunkJIT::regT0, SpecializedThunkJIT::fpRegT2);
> +        jit.subDouble(SpecializedThunkJIT::fpRegT1, SpecializedThunkJIT::fpRegT2, SpecializedThunkJIT::fpRegT0);

Note: I'm doing this instead of branching and subtracting a constant 1, because even in the non-subtracting case we would still need to move the result value to fpRegT0. I think this is probably fine (especially since neither branch is more likely than the other) but if I'm overlooking an obvious improvement please let me know. :)

[Saam Barati]

Comment on attachment 378145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378145&action=review

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Our Math.round implementation goes in the wrong direction for double values like 0.49999999999999994.

🤔

Gotta keep developers on their toes

> Source/JavaScriptCore/ChangeLog:9
> +        This requires just a subtle adjustment for three of our four versions; only baseline JIT needed a full rewrite. 

why didn't it work? Would be good to say why it didn't work too. Writing out why it didn't work would be helpful for folks referring back to this.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2955
> +                LValue scratch = m_out.doubleSub(integerValue, m_out.constDouble(0.5));

style nit: we can probably pick a better name than scratch

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1009
>> +        jit.subDouble(SpecializedThunkJIT::fpRegT1, SpecializedThunkJIT::fpRegT2, SpecializedThunkJIT::fpRegT0);
> 
> Note: I'm doing this instead of branching and subtracting a constant 1, because even in the non-subtracting case we would still need to move the result value to fpRegT0. I think this is probably fine (especially since neither branch is more likely than the other) but if I'm overlooking an obvious improvement please let me know. :)

my blind guess is the branch might be faster than convertInt32ToDouble, but I haven't benchmarked it.

[Ross Kirsling]

Created attachment 378241 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 378241 [details]
Patch for landing

Clearing flags on attachment: 378241

Committed r249597: <https://trac.webkit.org/changeset/249597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55131492>