Closed Bug 1346028 Opened 5 years ago Closed 5 years ago

Ion: Implement INITELEM_INC

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: evilpie, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

INITELEM_INC is used for spread literals in arrays like [1, 2, ...[3, 4, 5]]. This is used in six-speed-spread-literal-es6.
I think Ted is implementing that in bug 1338920.
Depends on: 1338920
Priority: -- → P3
Assignee: nobody → 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.
Ah, I see what you mean. I'll break out the patch for this separately.
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?
This is the naive version using VM calls to get it landed. I've opened Bug 1347972 for followup optimization work.
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.
Attachment #8848123 - Flags: review?(hv1989)
Attachment #8848123 - Flags: review?(hv1989) → review?(jdemooij)
Attachment #8849341 - Flags: review?(jdemooij)
Blocks: 1338920
No longer depends on: 1338920
Comment on attachment 8848123 [details]
Bug 1346028 - Allow MCallInitElementArray to take dynamic index

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

Makes sense.
Attachment #8848123 - Flags: review?(jdemooij) → review+
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.
Attachment #8849341 - Flags: review?(jdemooij) → review+
Pushed by tcampbell@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/3d08ee7b2eb8
https://hg.mozilla.org/mozilla-central/rev/4805c6c37555
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.