854400 - UnsafeSetElement does not handle double conversion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

I believe that the inlining for UnsafeSetElement() is not properly handling double conversion.

On IRC, jandem writes:

[09:59:31] <jandem> nmatsakis: yes you have to ensure int32 values written to the array are converted to double if needed
[10:00:32] <jandem> nmatsakis: something like TypeOracle::elementWriteNeedsDoubleConversion, if that's |true| then insert an MToDouble instruction, like IonBuilder::jsop_setelem_dense

One tricky part is that creating a simple testcase for intrinsics is difficult since they cannot be directly invoked.

Comment 1

[Niko Matsakis [:nmatsakis]]

It turns out intrinsics can be accessed from within the shell, so it shouldn't be too hard to devise a test.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: patch

Not sure how to write a testcase for this, since we can't test type tags.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Rebase shu's patch and reuse jsop_setelem_dense path when inlining

Carrying over r+ from bhackett, which was given on attachment 746701 [details] [diff] [review] (accidentally added to bug 833898)

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Reuse jsop_setelem_typed when inlining UnsafeSetElement applied to a typed array

Comment 5

[Niko Matsakis [:nmatsakis]]

Attachment: Disable double conversion during parallel exec

I am not 100% sure how to create a targeted test case for this, but I do know that double conversion was blocking the test that I will be adding in bug 865931.

Comment 6

[Niko Matsakis [:nmatsakis]]

Try run for these patches (also including patch from bug 865931) looks green (at least for SM tests, didn't run the rest): https://tbpl.mozilla.org/?tree=Try&rev=ec8d7228222b

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 747231 [details] [diff] [review]
Disable double conversion during parallel exec

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

::: js/src/ion/IonBuilder.cpp
@@ +6226,5 @@
>      // If we can load the element as a definite double, make sure to check that
>      // the array has been converted to homogenous doubles first.
> +    //
> +    // NB: We disable this optimization in parallel execution mode
> +    // because it inherently not threadsafe (how do you convert the

Nit: s/it/it's

Comment 8

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/378d4fe10c1f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fee19aa62f5d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/89426b7c3b3b

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/378d4fe10c1f
https://hg.mozilla.org/mozilla-central/rev/fee19aa62f5d
https://hg.mozilla.org/mozilla-central/rev/89426b7c3b3b