Bug 67553

Summary:

ThunkGenerators does not convert positive double zero into integer zero

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
the patch	none
the patch - 32 bit as well	none

Filip Pizlo

Reported 2011-09-02 22:06:11 PDT

ThunkGenerators does not convert positive double zero into integer zero, particularly when returning from floor, ceil, and round. This means that doing Math.floor(2/1) does not return an integer as some benchmarks expect. This omission is due to conservatism when dealing with double zero. A double zero could be negative, in which case converting to an integer would be wrong. But because the ThunkGenerators return the result in a GPR after boxing, it's possible to easily detect positive double zero. The first step of boxing is moving the double into a GPR. When the value is in a GPR, a positive double zero is represented by all bits being identically 0. Thus, a branchTestPtr(Zero, regT0) is all that is needed to detect positive double zero and convert it to an integer zero.

Attachments
the patch (1.54 KB, patch) 2011-09-02 22:11 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - 32 bit as well (2.07 KB, patch) 2011-09-03 02:13 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2011-09-02 22:11:17 PDT

Created attachment 106246 [details] the patch

Filip Pizlo

Comment 2 2011-09-02 22:27:28 PDT

Comment on attachment 106246 [details] the patch All tests pass.

Gavin Barraclough

Comment 3 2011-09-03 01:43:29 PDT

Hey Filip, I haven't looked closely yet, but we really should make this work for JSVALUE32_64 too, if it doesn't do so already.

Filip Pizlo

Comment 4 2011-09-03 01:46:40 PDT

(In reply to comment #3) > Hey Filip, I haven't looked closely yet, but we really should make this work for JSVALUE32_64 too, if it doesn't do so already. It's not clear to me that this will be performance-neutral in JSVALUE32_64. On 64-bit it's a single branch. On JSVALUE32_64 it would be two branches. I'll mess around with it though.

Filip Pizlo

Comment 5 2011-09-03 02:13:40 PDT

Created attachment 106258 [details] the patch - 32 bit as well

Gavin Barraclough

Comment 6 2011-09-03 14:57:16 PDT

Comment on attachment 106258 [details] the patch - 32 bit as well Awesome, cheers for covering 32_64 too Phil.

WebKit Review Bot

Comment 7 2011-09-03 22:42:54 PDT

Comment on attachment 106258 [details] the patch - 32 bit as well Clearing flags on attachment: 106258 Committed r94500: <http://trac.webkit.org/changeset/94500>

WebKit Review Bot

Comment 8 2011-09-03 22:42:58 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.