1337773 - Add an Ion IC for JSOP_IN

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Jan de Mooij [:jandem]]

We never had an Ion IC for JSOP_IN, but it should be very easy to add one now after Ted's work converting the Baseline IC to CacheIR.

Comment 1

[Tom S. (please needinfo tschuster)]

The testcase in bug 1342797 spends a lot of time calling a the VM function for "In" from ion. This happens because the in expression inside the main loop of ArrayConcat isn't being optimized.

Comment 2

[Tom S. (please needinfo tschuster)]

This would be almost identical to this patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d036ef05e8. Maybe we should look into unify hasOwnProperty an |in|.

Comment 3

[Ted Campbell [:tcampbell]]

Thanks for figuring it out, Tom. Saved me a bunch of debug time. I have a version working for JSOP_IN that I still want to refactor and polish a few things, but try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42a80a7ef39ef2f2af4b573956d2094213275df0

Comment 4

[Ted Campbell [:tcampbell]]

Gonna pause on this for a few days until Bug 1344469 settles. Hopefully can combine the two ICs. I have a working version without code sharing if urgent.

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Use IC for JSOP_IN in Ion

This should mostly be working. Will likely do some refactoring and enhancements before it lands though.

Comment 6

[Ted Campbell [:tcampbell]]

Note to self: Need to fix LoadDenseElementHoleExistsResult to support typed register. See jit-test/basic/array-copyWithin

Comment 7

[Ted Campbell [:tcampbell]]

Attachment: Bug 1337773 - Add IonInIC

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

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: Bug 1337773 - Use IonInIC for MIn and rename to MInCache

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

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8863389 [details]
Bug 1337773 - Add IonInIC

https://reviewboard.mozilla.org/r/135154/#review138464

\o/

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8863390 [details]
Bug 1337773 - Use IonInIC for MIn and rename to MInCache

https://reviewboard.mozilla.org/r/135156/#review138466

::: js/src/jit/MIR.h:12498
(Diff revision 1)
> -class MIn
> +class MInCache
>    : public MBinaryInstruction,
> -    public MixPolicy<BoxPolicy<0>, ObjectPolicy<1> >::Data
> +    public MixPolicy<CacheIdPolicy<0>, ObjectPolicy<1> >::Data
>  {
> -    MIn(MDefinition* key, MDefinition* obj)
> +    MInCache(MDefinition* key, MDefinition* obj)
>        : MBinaryInstruction(key, obj)

I was thinking it might make sense to match MHasOwnCache and add the object first, but for MInCache (key, obj) may be more natural so we can keep it like this.

Comment 11

[Ted Campbell [:tcampbell]]

In future it would be nice to make fix the order. Requires redoing the various getters (rhs/lhs of BinaryInstruction is very confusing when the original bytecode has key, obj). Was thinking that MInCache and MHasOwnCache should be merged (or at least share a base class) and then we can pursue further optimizations together in the MIR. We can open a follow-up bug (the refactoring might be a good-first-bug).

Comment 12

[Jan de Mooij [:jandem]]

Sounds good to me.

Comment 13

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 33e5d752ca41 -d c06620d52d5f: rebasing 393412:33e5d752ca41 "Bug 1337773 - Add IonInIC r=jandem"
merging js/src/jit/CodeGenerator.cpp
merging js/src/jit/IonCacheIRCompiler.cpp
merging js/src/jit/IonIC.cpp
warning: conflicts while merging js/src/jit/CodeGenerator.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging js/src/jit/IonCacheIRCompiler.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging js/src/jit/IonIC.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 14

[Ted Campbell [:tcampbell]]

Comment on attachment 8863389 [details]
Bug 1337773 - Add IonInIC

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

Comment 15

[Ted Campbell [:tcampbell]]

Comment on attachment 8863390 [details]
Bug 1337773 - Use IonInIC for MIn and rename to MInCache

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/212671d5e91e
Add IonInIC r=jandem
https://hg.mozilla.org/integration/autoland/rev/ba61a05620e4
Use IonInIC for MIn and rename to MInCache r=jandem

Comment 17

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/212671d5e91e
https://hg.mozilla.org/mozilla-central/rev/ba61a05620e4