Closed
Bug 965767
Opened 9 years ago
Closed 9 years ago
Ionmonkey ARM: correct the load-immediate-float32 instruction encoding
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: dougc, Assigned: dougc)
References
Details
Attachments
(1 file, 2 obsolete files)
69.18 KB,
patch
|
dougc
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
Address reviewer feedbac: clarify source comments. Carrying forward r+.
Attachment #8372712 -
Attachment is obsolete: true
Attachment #8372740 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/44212959e3b0
Flags: in-testsuite+
Keywords: checkin-needed
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/44212959e3b0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•