1346028 - Ion: Implement INITELEM_INC

Status: RESOLVED FIXED

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

Description

[Tom S]

INITELEM_INC is used for spread literals in arrays like [1, 2, ...[3, 4, 5]]. This is used in six-speed-spread-literal-es6.

Comment 1

[Hannes Verschore [:h4writer]]

I think Ted is implementing that in bug 1338920.

Comment 2

[Ted Campbell [:tcampbell]]

Yes, this is part of spreadcall work. I have a patch already, it just is difficult to test stand-alone because only spreadcall generates the opcode right now.

Comment 3

[Ted Campbell [:tcampbell]]

Ah, I see what you mean. I'll break out the patch for this separately.

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1346028 - Allow MCallInitElementArray to take dynamic index

Review commit: https://reviewboard.mozilla.org/r/121092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121092/

Comment 5

[Jan de Mooij [:jandem]]

Baseline compiles this as INITELEM and does the increment in the BaselineCompiler. Wouldn't it be easier and faster to do the same in IonBuilder? Or maybe we could change the bytecode emitter to emit JSOP_INITELEM + JSOP_ADD?

Comment 6

[Ted Campbell [:tcampbell]]

This is the naive version using VM calls to get it landed. I've opened Bug 1347972 for followup optimization work.

Comment 7

[Ted Campbell [:tcampbell]]

Comment on attachment 8848123 [details]
Bug 1346028 - Allow MCallInitElementArray to take dynamic index

Cancelling review. Will take another pass and look into how Baseline does it.

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: Bug 1346028 - Support JSOP_INITELEM_INC in Ion

Review commit: https://reviewboard.mozilla.org/r/122128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122128/

Comment 9

[Ted Campbell [:tcampbell]]

Comment on attachment 8848123 [details]
Bug 1346028 - Allow MCallInitElementArray to take dynamic index

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121092/diff/1-2/

Comment 10

[Ted Campbell [:tcampbell]]

Comment on attachment 8848123 [details]
Bug 1346028 - Allow MCallInitElementArray to take dynamic index

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121092/diff/2-3/

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8849341 [details]
Bug 1346028 - Support JSOP_INITELEM_INC in Ion

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122128/diff/1-2/

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8848123 [details]
Bug 1346028 - Allow MCallInitElementArray to take dynamic index

https://reviewboard.mozilla.org/r/121092/#review125852

Makes sense.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8849341 [details]
Bug 1346028 - Support JSOP_INITELEM_INC in Ion

https://reviewboard.mozilla.org/r/122128/#review125860

Great to have Ion support for this!

::: js/src/jit/IonBuilder.cpp:6115
(Diff revision 2)
> +    MDefinition* id = current->pop();
> +    MDefinition* obj = current->peek(-1);
> +
> +    bool emitted = false;
> +
> +    MAdd* nextId = MAdd::New(alloc(), id, constantInt(1), MIRType::Int32);

The code in BaselineCompiler::emit_JSOP_INITELEM_INC assumes the value is int32, but Ion sometimes decides to store integers as doubles so please add some code to emit_JSOP_INITELEM_INC to double check the Value we see there is int32. Something like:

```
#ifdef DEBUG
    Label isInt32;
    masm.branchTestInt32(Assembler::Equal, indexAddr, &isInt32);
    masm.assumeUnreachable(...);
    masm.bind(&isInt32);
#endif
```

::: js/src/jit/IonBuilder.cpp:8975
(Diff revision 2)
>          }
>      }
>  
> -    if (PropertyWriteNeedsTypeBarrier(alloc(), constraints(), current,
> +    // Emit a type barrier if a write would update the typeset. Holes may be
> +    // written during array intitialization, but this doesn't affect typeset.
> +    if (value->type() != MIRType::MagicHole &&

To match the interpreter, I don't think we should write an explicit hole value (see InitArrayElemOperation and SetPropIRGenerator::tryAttachSetDenseElementHole checking for the hole value). I'd prefer not calling initOrSetElemTryDense when we have the hole value - we can fall back to the IC code for that edge case.

Comment 14

[Ted Campbell [:tcampbell]]

Comment on attachment 8849341 [details]
Bug 1346028 - Support JSOP_INITELEM_INC in Ion

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122128/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d08ee7b2eb8
Allow MCallInitElementArray to take dynamic index r=jandem
https://hg.mozilla.org/integration/autoland/rev/4805c6c37555
Support JSOP_INITELEM_INC in Ion r=jandem

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3d08ee7b2eb8
https://hg.mozilla.org/mozilla-central/rev/4805c6c37555