Bug 67553 - ThunkGenerators does not convert positive double zero into integer zero
Summary: ThunkGenerators does not convert positive double zero into integer zero
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-02 22:06 PDT by Filip Pizlo
Modified: 2011-09-03 22:42 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 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.
Comment 1 Filip Pizlo 2011-09-02 22:11:17 PDT
Created attachment 106246 [details]
the patch
Comment 2 Filip Pizlo 2011-09-02 22:27:28 PDT
Comment on attachment 106246 [details]
the patch

All tests pass.
Comment 3 Gavin Barraclough 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.
Comment 4 Filip Pizlo 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.
Comment 5 Filip Pizlo 2011-09-03 02:13:40 PDT
Created attachment 106258 [details]
the patch - 32 bit as well
Comment 6 Gavin Barraclough 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.
Comment 7 WebKit Review Bot 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>
Comment 8 WebKit Review Bot 2011-09-03 22:42:58 PDT
All reviewed patches have been landed.  Closing bug.