Closed Bug 916662 Opened 6 years ago Closed 6 years ago

Fix Float32 assertions on ARM

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch arm-assertions.patch (obsolete) — Splinter Review
Following bug 915495, comment 20, some assertions showed up on ARM after the Float32 landing.

The assertions during Lowering weren't taking into account whether the frontend can generate Float32 or not.
 
Doug, could you check that it solves most of the jit-test failures on ARM, please? I will flag for review only if it's the case.
Attachment #805145 - Flags: feedback?(dtc-moz)
(In reply to Benjamin Bouvier [:bbouvier] from comment #0)
> Created attachment 805145 [details] [diff] [review]
> arm-assertions.patch
...  
> Doug, could you check that it solves most of the jit-test failures on ARM,
> please? I will flag for review only if it's the case.

Looks like a useful improvement, thank you.  Re-ran all the failing tests
in this class and they all either pass or fail for other reasons now.  

In particular:

tests/basic/testTypedArrays.js now crashes trying to re-target a near branch.

tests/jaeger/testSetTypedFloatArray.js now crashes due to failure of the
constant pools which bails out and cause a subsequent assertion failure.

tests/ion/setelem-float32-typedarray-ic.js is still failing with numerical
errors.
Attached patch fixSplinter Review
- Fixes assertions by checking that we can actually emit Float32.
- Fixes a case where a conversion to Float was done twice, on ARM (IonCaches).
Attachment #805145 - Attachment is obsolete: true
Attachment #805145 - Flags: feedback?(dtc-moz)
Attachment #805586 - Flags: review?(shu)
Comment on attachment 805586 [details] [diff] [review]
fix

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

Looks good!

An aside: I wonder if we should do some refactoring to unify the use of ScalarTypeRepresentation in typed array (and soon, typed object) related code to determine the action over MIRType?
Attachment #805586 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/ccfb71f96791
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 805586 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression of bug 915495
User impact if declined: correctness issues on websites that use Float32Arrays on ARM (phones and tablets), that can lead to visual glitches or crashes.
Testing completed (on m-c, etc.): no problem on m-c
Risk to taking this patch (and alternatives if risky): very low, if not no risk at all
String or IDL/UUID changes made by this patch: N/A
Attachment #805586 - Flags: approval-mozilla-aurora?
Attachment #805586 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.