Closed Bug 1337773 Opened 3 years ago Closed 3 years ago

Add an Ion IC for JSOP_IN


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




Tracking Status
firefox55 --- fixed


(Reporter: jandem, Assigned: tcampbell)


(Blocks 2 open bugs)


(Whiteboard: [qf:p3])


(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: 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:
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

Attachment #8863389 - Flags: review?(jdemooij) → review+
Comment on attachment 8863390 [details]
Bug 1337773 - Use IonInIC for MIn and rename to MInCache

::: 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)
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.