878510 - IonMonkey: (ARM) Add support for real negative zero check in convertDoubleToint32

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

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 ;)

Comment 1

[Hannes Verschore [:h4writer]]

And I also think it can remove the ss-cordic regression...

Comment 2

[Douglas Crosher [:dougc]]

(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.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Add negative zero check

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.)

Comment 4

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 757172 [details] [diff] [review]
Add negative zero check

Review of attachment 757172 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.

Comment 5

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 757172 [details] [diff] [review]
Add negative zero check

Review of attachment 757172 [details] [diff] [review]:
-----------------------------------------------------------------

still looks good.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Add negative zero check with coditional ins

Marty asked this over IRC. Same improvement, but now with conditional instructions instead of branches.

Comment 7

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5483b4f99fc

Pushed the r+ed one

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5483b4f99fc