1420172 - Scalar Replacement: Allow writing to MNewArrayCopyOnWrite arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Tom S [:evilpie]]

I didn't implement this in bug 1288392, because it seemed to make some things simpler. This might just work now however, or maybe initFromTemplateObject needs to be adjusted.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This implements scalar replacement for stores to COW arrays. We now also support MConvertElementsToDoubles.

With these changes the raytracer in bug 1425687 is > 2x faster.

Comment 2

[Tom S [:evilpie]]

Comment on attachment 8938415 [details] [diff] [review]
Patch

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

Looks pretty good. Are writes guaranteed to be preceded by CopyElementsOnWrite?

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8938415 [details] [diff] [review]
Patch

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

Everything looks good except for the next comment:

::: js/src/jit/ScalarReplacement.cpp
@@ +1438,5 @@
> +            oom_ = true;
> +            return;
> +        }
> +
> +        state_->setCopyElementsForWrite();

This flag should be updated in ArrayMemoryView::mergeIntoSuccessorState, otherwise the following test case might fail:

function arrayCopyWrite1(i) {
    // (block 0)
    var a = [1, 1];
    if (i % 2 == 0)
        // (block 1)
        a[0] = a[0] + 1;
    // (block 2)
    assertEq(a[0], 1);
    assertEq(a[1], 2);
    assertRecoveredOnBailout(a, true);
    resumeHere();
    return a.length;
}


The problem being that block 0, will create block 2, MArrayState without the copy flag. When the mutated data from block 1 is merged into block 2, the function mergeIntoSuccessorState will add the Phi nodes but forget to set the copy flag on block 2 MArrayState.

Note, the same applies, if the array mutation is nested in a for-loop:

function arrayCopyWrite2(i) {
    var a = [1, 1];
    for (var x = 0; x < 2; x++) {
        if (x % 2 == 1)
            resumeHere(); // (MArrayState needs the Copy flag)
        else
            a[0] = a[0] + 1;
    }
    assertEq(a[0], 1);
    assertEq(a[1], 2);
    assertRecoveredOnBailout(a, true);
    return a.length;
}

The easiest way, would be to make the Copy flag be a boolean operand of the MArrayState vector which would then be merged as a Phi operand by mergeIntoSuccessorState, which should solve both issues seen above.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Good catch! This adds the copyElementsForWrite flag as operand to MArrayState and adds some tests.

Comment 5

[Jan de Mooij [:jandem]]

Another option is to check in RArrayState::recover whether we have an element that's != the COW element and then call maybeCopyElementsForWrite. That way we don't have to track copyElementsForWrite at all. I can do that too..

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Another option is to check in RArrayState::recover whether we have an
> element that's != the COW element and then call maybeCopyElementsForWrite.
> That way we don't have to track copyElementsForWrite at all. I can do that
> too..

I guess this would work, and even do some de-duplication.
I think this is even better than keeping track of the copy flag.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

This works and is a much simpler patch.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceb61da04fd8
Make scalar replacement of arrays work in more cases (writes to COW elements, double elements). r=nbp

Comment 10

[Stefan Hindli [:stefan_hindli]]

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