965767 - Ionmonkey ARM: correct the load-immediate-float32 instruction encoding

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

When the VFPv3 feature is detected the ARM compiler backend tries to use the vimm instruction to load an immediate float32 value, see ma_vimm_f32.  However it does not take into account that the encoding of the immediate value differs between vimm.f64 and vimm.f32 and it is loading incorrect values for some immediate float32 values.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Correct the load-immediate-float32 instruction encoding

This patch renames the current VFPImm class to VFPDoubleImm, and adds a new class VFPFloat32Imm to handle the vimm instruction's float32 immediate encoding.

Comment 2

[Douglas Crosher [:dougc]]

Comment on attachment 8368301 [details] [diff] [review]
Correct the load-immediate-float32 instruction encoding

Seeing potential new errors with this patch applied.  All the jit-tests pass, and it fixed the test failure found.  But best keep analysing the problems and sort it all out first.

Comment 3

[Douglas Crosher [:dougc]]

Comment on attachment 8368301 [details] [diff] [review]
Correct the load-immediate-float32 instruction encoding

The glitch seen turned out to be due to unrelated changes in the WebGL texture support.  Looks good now, after taking this into account.

Comment 4

[Douglas Crosher [:dougc]]

Attachment: Correct the load-immediate-float32 instruction encoding

Thank you for the feedback on IRC.  It had not occurred to me that the double path could still be used - a source code comment has been added to point this out.  The fix is now much simpler - just check that the double low bits are zero. More tests have also been added: example values that were being incorrectly encoded, and other edge cases.

Comment 5

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8372712 [details] [diff] [review]
Correct the load-immediate-float32 instruction encoding

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

That is quite the test case.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +1529,5 @@
>          }
>  
> +        // Note that the vimm immediate float32 instruction encoding differs from the
> +        // vimm immediate double encoding, but this difference matches the difference
> +        // in the floating point formats, so it is possible to round to a double and

minor nit, float32 to double conversions aren't lossy, it isn't rounding, it is just a conversion

Comment 6

[Douglas Crosher [:dougc]]

Attachment: Correct the load-immediate-float32 instruction encoding

Address reviewer feedbac: clarify source comments.

Carrying forward r+.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44212959e3b0

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/44212959e3b0