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
Comment on attachment 339080 [details] Patch This doesn't address the bug in our JITs as well. Check out DFG/FTL ArithRound
Created attachment 378145 [details] Patch
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. :)
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.
Created attachment 378241 [details] Patch for landing
Comment on attachment 378241 [details] Patch for landing Clearing flags on attachment: 378241 Committed r249597: <https://trac.webkit.org/changeset/249597>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55131492>