Ion GetProperty IC should be used when the input is a Value

RESOLVED FIXED in Firefox 54

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [qf-])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Ion ICs are currently only used when the input is an object. This covers the majority of cases, but we have seen benchmarks (Octane-crypto is one of them, IIRC) that do something like x.length/x.toString()/x[i] where x is {string, object}.

Once bug 1322093 is fixed, it will be easy to fix this: we can upgrade the object-Register in IonGetPropertyIC to a TypedOrValueRegister and make the C++ fallback function take a Value, and then we should automatically emit the right guards and regalloc stuff.

Filing this now so I don't forget about it.
If I recall correctly, this case appears in react code as well. (see Bug 1301690)
(Assignee)

Comment 2

2 years ago
I had some WIP patches for this that worked fine. I'll try to get them landed this cycle.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
This patch changes the GetPropertyIC to take a TypedOrValueRegister as input, so now it can be used for any JSOP_GETPROP. We no longer need the shared stub or MCallGetProperty (there are some other places that use this so we can't remove it).

Later we can do something similar for JSOP_GETELEM.
Attachment #8829782 - Flags: review?(hv1989)
For register usage wouldn't it be better to have an object and a value version of GetPropertyCache?
(Assignee)

Comment 5

2 years ago
(In reply to Tom Schuster [:evilpie] from comment #4)
> For register usage wouldn't it be better to have an object and a value
> version of GetPropertyCache?

We use a TypedOrValueRegister, so it's either an object register (as before) or a boxed Value.
Comment on attachment 8829782 [details] [diff] [review]
Part 1 - GetProp

Review of attachment 8829782 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: js/src/jit/Lowering.cpp
@@ +3656,5 @@
>          defineBox(lir, ins);
>          assignSafepoint(lir, ins);
>      } else {
>          LGetPropertyCacheT* lir =
> +            new(alloc()) LGetPropertyCacheT(useBoxOrTypedOrConstant(value, /* useConst = */ false),

Shouldn't we introduce "useBoxOrTyped" for this?
Attachment #8829782 - Flags: review?(hv1989) → review+

Comment 7

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb0de976a39e
part 1 - Use Ion GetPropertyIC also when the input is a boxed Value. r=h4writer
(Assignee)

Comment 8

2 years ago
(In reply to Hannes Verschore [:h4writer] from comment #6)
> Shouldn't we introduce "useBoxOrTyped" for this?

Good idea. I added useBoxOrTyped and changed useBoxOrTypedOrConstant to call that in the not-a-constant case.
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 10

2 years ago
There's more to do here (for JSOP_GETELEM) but let's close this one to make it easier to track this.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.