Closed
Bug 878510
Opened 11 years ago
Closed 11 years ago
IonMonkey: (ARM) Add support for real negative zero check in convertDoubleToint32
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: h4writer, Assigned: h4writer)
Details
Attachments
(2 files)
1.91 KB,
patch
|
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
1.83 KB,
patch
|
Details | Diff | Splinter Review |
In bug 878019 I stated that we are bailing in ARM, for a reason that doesn't happen in x86. I've now been able to reduce it to "convertDoubleToint32". In that function ARM will bail for every "0" encounter. While x86 only bails for "-0.0". so for parity with x86 and also because it will bring another increase from 6300 to 7900 to raytrace, we should implement this. (Then we will beat v8 on octane-raytrace). My knowledge about ARM is really basic (non-existant), but I googled already a lot and didn't find a real easy way to do it ... Also the comments in the code suggests it isn't that easy. So I don't know a way to do it in ARM. My next step would be to just compile the snippit and look how gcc compiles a negative zero check. On the other hand, Marty if you have an idea feel free to jump in ;)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(mrosenberg)
Assignee | ||
Comment 1•11 years ago
|
||
And I also think it can remove the ss-cordic regression...
Comment 2•11 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #0) > In bug 878019 I stated that we are bailing in ARM, for a reason that doesn't > happen in x86. I've now been able to reduce it to "convertDoubleToint32". In > that function ARM will bail for every "0" encounter. While x86 only bails > for "-0.0". ... > > My knowledge about ARM is really basic (non-existant), but I googled already > a lot and didn't find a real easy way to do it ... Also the comments in the > code suggests it isn't that easy. So I don't know a way to do it in ARM. My > next step would be to just compile the snippit and look how gcc compiles a > negative zero check. On the other hand, Marty if you have an idea feel free > to jump in ;) Transfer it to a general purpose register and then check the sign bit. The VMOV instruction can move just the high half, with the sign bit, and perhaps the scratch register is free to use. The ARM backend appears to have missed out on support in Bug 863349 which might also be causing some differences in bailouts for double to integer operations.
Assignee | ||
Comment 3•11 years ago
|
||
Thanks Douglas for some pointers. Looking through the code I actually found already a place where we were checking for negative zero (floor). So I just copied that code. This improves raytrace to 7700 on arm. (I hope it fixes FFOs ss-cordic too, but I can't test it atm. I'll check after landing.)
Assignee: general → hv1989
Attachment #757172 -
Flags: review?(mrosenberg)
Flags: needinfo?(mrosenberg)
Comment 4•11 years ago
|
||
Comment on attachment 757172 [details] [diff] [review] Add negative zero check Review of attachment 757172 [details] [diff] [review]: ----------------------------------------------------------------- looks good.
Comment 5•11 years ago
|
||
Comment on attachment 757172 [details] [diff] [review] Add negative zero check Review of attachment 757172 [details] [diff] [review]: ----------------------------------------------------------------- still looks good.
Attachment #757172 -
Flags: review?(mrosenberg) → review+
Assignee | ||
Comment 6•11 years ago
|
||
Marty asked this over IRC. Same improvement, but now with conditional instructions instead of branches.
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5483b4f99fc Pushed the r+ed one
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c5483b4f99fc
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•