Ion: Implement INITELEM_INC

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: evilpie, Assigned: tcampbell)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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)

Updated

2 years ago
Assignee: nobody → tcampbell
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
Ah, I see what you mean. I'll break out the patch for this separately.
Comment hidden (mozreview-request)
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?
(Assignee)

Comment 6

2 years ago
This is the naive version using VM calls to get it landed. I've opened Bug 1347972 for followup optimization work.
(Assignee)

Comment 7

2 years ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8848123 - Flags: review?(hv1989) → review?(jdemooij)
Attachment #8849341 - Flags: review?(jdemooij)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1338920
No longer depends on: 1338920

Comment 12

2 years ago
mozreview-review
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 13

2 years ago
mozreview-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+
Comment hidden (mozreview-request)

Comment 15

2 years ago
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

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d08ee7b2eb8
https://hg.mozilla.org/mozilla-central/rev/4805c6c37555
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.