Closed Bug 1337773 Opened 3 years ago Closed 3 years ago

Add an Ion IC for JSOP_IN

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jandem, Assigned: tcampbell)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [qf:p3])

Attachments

(2 files, 1 obsolete file)

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.
Priority: -- → P2
Assignee: nobody → tcampbell
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.
Blocks: 1342797
Whiteboard: [qf:p2]
Whiteboard: [qf:p2] → [qf:p3]
Blocks: emberperf
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|.
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
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.
Depends on: 1357759
Blocks: 1357759
No longer depends on: 1357759
Attached patch Use IC for JSOP_IN in Ion (obsolete) — Splinter Review
This should mostly be working. Will likely do some refactoring and enhancements before it lands though.
Note to self: Need to fix LoadDenseElementHoleExistsResult to support typed register. See jit-test/basic/array-copyWithin
Depends on: 1359952
Attachment #8859629 - Attachment is obsolete: true
Comment on attachment 8863389 [details]
Bug 1337773 - Add IonInIC

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

\o/
Attachment #8863389 - Flags: review?(jdemooij) → review+
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.
Attachment #8863390 - Flags: review?(jdemooij) → review+
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).
Sounds good to me.
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)
https://hg.mozilla.org/mozilla-central/rev/212671d5e91e
https://hg.mozilla.org/mozilla-central/rev/ba61a05620e4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.