Closed Bug 965767 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: dougc, Assigned: dougc)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
This patch renames the current VFPImm class to VFPDoubleImm, and adds a new class VFPFloat32Imm to handle the vimm instruction's float32 immediate encoding.
Attachment #8368301 - Flags: review?(mrosenberg)
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.
Attachment #8368301 - Flags: review?(mrosenberg)
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.
Attachment #8368301 - Flags: review?(mrosenberg)
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.
Attachment #8368301 - Attachment is obsolete: true
Attachment #8368301 - Flags: review?(mrosenberg)
Attachment #8372712 - Flags: review?(mrosenberg)
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
Attachment #8372712 - Flags: review?(mrosenberg) → review+
Address reviewer feedbac: clarify source comments.

Carrying forward r+.
Attachment #8372712 - Attachment is obsolete: true
Attachment #8372740 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/44212959e3b0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.