RESOLVED FIXED 67553
ThunkGenerators does not convert positive double zero into integer zero 
https://bugs.webkit.org/show_bug.cgi?id=67553
Summary ThunkGenerators does not convert positive double zero into integer zero
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
DetailsFormatted DiffDiff
the patch - 32 bit as well (2.07 KB, patch)
2011-09-03 02:13 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
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.