916662 - Fix Float32 assertions on ARM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: arm-assertions.patch

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.

Comment 1

[Douglas Crosher [:dougc]]

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

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix

- Fixes assertions by checking that we can actually emit Float32.
- Fixes a case where a conversion to Float was done twice, on ARM (IonCaches).

Comment 3

[Shu-yu Guo [: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?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f01213f71771