[webkit-reviews] review granted: [Bug 185115] Math.round() produces wrong result for value prior to 0.5 : [Attachment 378145] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 185115: Math.round() produces wrong result for value prior to 0.5
https://bugs.webkit.org/show_bug.cgi?id=185115

Attachment 378145: Patch

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




--- Comment #6 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 378145
  --> https://bugs.webkit.org/attachment.cgi?id=378145
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.