Closed Bug 729099 Opened 12 years ago Closed 12 years ago

IonMonkey: GETPROP IC: support properties on the prototype chain

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Need this for v8-richards.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Mostly similar to JM, and passes all tests with --ion -n and --ion-eager -n.

With this patch + bug 728940, we get closer to JM+TI on v8-richards:

Ion+TI (old):  4280
JM:            6284
Ion+TI (new): 10760
JM+TI:        12214
Attachment #599303 - Flags: review?(bhackett1024)
Comment on attachment 599303 [details] [diff] [review]
Patch

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

::: js/src/ion/IonCaches.cpp
@@ +88,5 @@
>  static const size_t MAX_STUBS = 16;
>  
> +static void
> +GeneratePrototypeGuards(JSContext *cx, MacroAssembler &masm, JSObject *obj, JSObject *holder,
> +                        Register objectReg, Label *failures)

It would be really good if we could keep the object register immutable, and keep using useRegister for the GetProperty cache.  The motive here was that if there are a bunch of cached property accesses on the same object, the register should be reusable instead of needing to be regenerated for each access.  I don't think that doing this (as we already do for accesses loading from a dynamic property index) isn't that big.

@@ +104,5 @@
> +        if (pobj->hasUncacheableProto()) {
> +            if (pobj->hasSingletonType()) {
> +                types::TypeObject *type = pobj->getType(cx);
> +                masm.loadPtr(ImmWord(&type->proto), objectReg);
> +                masm.branchPtr(Assembler::NotEqual, objectReg, ImmGCPtr(pobj->getProto()), failures);

It would be nice if this could use AbsoluteAddress, like the JM version.  Less instruction traffic on x86 (and ARM?)

@@ +107,5 @@
> +                masm.loadPtr(ImmWord(&type->proto), objectReg);
> +                masm.branchPtr(Assembler::NotEqual, objectReg, ImmGCPtr(pobj->getProto()), failures);
> +            } else {
> +                masm.loadPtr(ImmWord(pobj->addressOfType()), objectReg);
> +                masm.branchPtr(Assembler::NotEqual, objectReg, ImmGCPtr(pobj->type()), failures);

Ditto.

@@ -114,5 @@
> -            masm.Push(slotsReg);
> -            restoreSlots = true;
> -        } else {
> -            slotsReg = output().typedReg().gpr();
> -        }

As above, I think this stuff should stay and the code should be restructured to get a scratch register if either the property is on the dynamic array or a proto walk is needed.
Attachment #599303 - Flags: review?(bhackett1024)
Attached patch Patch v2Splinter Review
- Adds an AbsoluteAddress structure. It's a lot like ImmWord, but what I like about it is that it's clear that the address will be dereferenced. I verified that the code we generate using it in GeneratePrototypeGuards is identical to JM.

- The object register is no longer writable. What's most complicated here is that there are two paths where we have to restore the register (normal rejoin path and the failure path).

- I left a question (comment marked with XXX) related to a moving GC, do we have to use ImmGCPtr(pobj) there instead?
Attachment #599303 - Attachment is obsolete: true
Attachment #599599 - Flags: review?(bhackett1024)
Comment on attachment 599599 [details] [diff] [review]
Patch v2

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

::: js/src/ion/IonCaches.cpp
@@ +112,5 @@
> +                               failures,
> +                               scratchReg);
> +            } else {
> +                // XXX: Is it safe to bake in pobj->addressOfType() with
> +                // a moving GC?

This is safe to do, if pobj is not in the nursery.  If pobj was in the nursery, we could either skip generating the stub or mark the IC for purging on the next nursery collection.  I think the former will suffice here and in other cases where ICs or the compiler itself wants to bake in object-related addresses.

@@ +133,5 @@
> +    Label failures;
> +    masm.branchPtr(Assembler::NotEqual,
> +                   Address(object(), JSObject::offsetOfShape()),
> +                   ImmGCPtr(obj->lastProperty()),
> +                   &failures);

Can the branchPtrWithPatch still be used if obj == holder?  Will save a jmp in the usual case that an 'own' property is being tested.  I think it's fine to call branchPtrWithPatch even if obj != holder and just ignore the patch offset, to avoid code bloat.

@@ +165,5 @@
> +        masm.movePtr(ImmGCPtr(holder), holderReg);
> +        masm.branchPtr(Assembler::NotEqual,
> +                       Address(holderReg, JSObject::offsetOfShape()),
> +                       ImmGCPtr(holder->lastProperty()),
> +                       &prototypeFailures);

Can the assembler branchPtr on an absolute address here?
Attachment #599599 - Flags: review?(bhackett1024) → review+
> ::: js/src/ion/IonCaches.cpp
> @@ +112,5 @@
> > +                               failures,
> > +                               scratchReg);
> > +            } else {
> > +                // XXX: Is it safe to bake in pobj->addressOfType() with
> > +                // a moving GC?
> 
> This is safe to do, if pobj is not in the nursery.  If pobj was in the
> nursery, we could either skip generating the stub or mark the IC for purging
> on the next nursery collection.  I think the former will suffice here and in
> other cases where ICs or the compiler itself wants to bake in object-related
> addresses.

That sounds fine. I'd prefer if we just treated all gc pointers as relocatable - we could do it here even if short on registers (on x86 we get absolute addressing, x64/arm have scratch - this can be abstracted in masm) - but if it's difficult, restricting the IC and having a comment (like visitRecompileCheck) is fine.
(In reply to Brian Hackett (:bhackett) from comment #4)
> 
> Can the branchPtrWithPatch still be used if obj == holder?

Good idea, done.

Also changed AbsoluteAddress to ImmGCPtr as suggested in comment 5. 

http://hg.mozilla.org/projects/ionmonkey/rev/ebca799b0a92
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.