Last Comment Bug 694169 - IonMonkey: Handle JSOP_CALLGNAME
: IonMonkey: Handle JSOP_CALLGNAME
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 705329 (view as bug list)
Depends on: 680315
Blocks: 684381 670484 708441
  Show dependency treegraph
 
Reported: 2011-10-12 15:30 PDT by Sean Stangl [:sstangl]
Modified: 2011-12-08 01:45 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (22.77 KB, patch)
2011-12-01 12:36 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch (25.08 KB, patch)
2011-12-02 03:50 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch v2 (27.80 KB, patch)
2011-12-02 05:50 PST, Jan de Mooij [:jandem]
marty.rosenberg: review+
sstangl: review+
Details | Diff | Splinter Review
Follow-up patch (17.01 KB, patch)
2011-12-07 02:20 PST, Jan de Mooij [:jandem]
sstangl: review+
Details | Diff | Splinter Review

Description Sean Stangl [:sstangl] 2011-10-12 15:30:49 PDT
Handling JSOP_CALLGNAME will greatly increase test coverage.
Comment 1 Jan de Mooij [:jandem] 2011-11-28 07:22:13 PST
*** Bug 705329 has been marked as a duplicate of this bug. ***
Comment 2 Jan de Mooij [:jandem] 2011-12-01 12:36:03 PST
Created attachment 578357 [details] [diff] [review]
WIP

The patch adds an MImplicitThis instruction which stores |undefined| if callee->parent == global and else bails out. Also fixes several bugs related to calls and inlining.

This introduces 5 new jit-test failures with --ion and a few more with --ion -n. Most are NYI asserts or asserts for arity mismatches when inlining. With the patch we can controlflow-recursive; perf is comparable to TI+JM and V8.

I think it's best to add the slow path later: it's not very common (especially with TI), OOL stub calls will need some work to not influence regalloc on the fast path and ObjShrink and CPG are on the horizon. Let me know if anybody disagrees.
Comment 3 Jan de Mooij [:jandem] 2011-12-02 03:50:27 PST
Created attachment 578542 [details] [diff] [review]
Patch

Sean, this patch always unwraps MPassArg for resume points. I tried to have it define its input like  you suggested, but for this to work it needs a new virtual reg for the payload. Just reusing the input does not solve the problem of VirtualRegisterOfPayload special-casing MBox but not MPassArg. Furthermore, this allows us to remove some MResumePoint methods used to unwrap MPassArg for inlined calls.

After rebasing there are now 3 jit-test failures with --ion. These are crashes caused by infinite recursion.
Comment 4 Jan de Mooij [:jandem] 2011-12-02 05:50:29 PST
Created attachment 578559 [details] [diff] [review]
Patch v2

Add ARM support. We already have LoadSlotV on ARM, only LoadSlotT is missing.
Comment 5 Sean Stangl [:sstangl] 2011-12-02 14:01:14 PST
Comment on attachment 578559 [details] [diff] [review]
Patch v2

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

No additional failures? We're in a lot better shape than I thought we would be! That's really great.

::: js/src/ion/IonBuilder.cpp
@@ +2044,5 @@
>      MDefinition *retvalDefn;
>      if (retvalDefns.length() > 1) {
>          // Need to create a phi to merge the returns together.
>          MPhi *phi = MPhi::New(bottom->stackDepth());
> +        bottom->addPhi(phi);

Good catch.

::: js/src/ion/MIR.cpp
@@ +720,5 @@
>      for (size_t i = 0; i < stackDepth(); i++) {
> +        MDefinition *def = block->getSlot(i);
> +        // We have to unwrap MPassArg: it's removed when inlining calls
> +        // and LStackArg does not define a value.
> +        if (def->isPassArg())

This looks fine.

::: js/src/ion/x86/CodeGenerator-x86.cpp
@@ +457,5 @@
> +    GlobalObject *global = gen->info().script()->global();
> +    masm.cmpl(Operand(callee, offsetof(JSObject, parent)), ImmGCPtr(global));
> +
> +    // TODO: OOL stub path.
> +    if (!bailoutIf(Assembler::NotEqual, lir->snapshot()))

If we take an OOL stub path, will we need a postSnapshot?

::: js/src/jit-test/tests/ion/callgname.js
@@ +7,5 @@
> +        y += g1(g1(i));
> +    }
> +    return y;
> +}
> +assertEq(f1(), 5150);

This test doesn't actually call g1 -- since g1 isn't compiled, and we can't yet call uncompiled functions, we'll take a bailout. To fix this, g1() must be called before f1() from the global script. Likewise with the rest of the code.
Comment 6 Marty Rosenberg [:mjrosenb] 2011-12-02 14:03:37 PST
Comment on attachment 578559 [details] [diff] [review]
Patch v2

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

I assume I'm only supposed to review the bits of code in CodeGenerator-arm.{cpp,h}

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +1385,5 @@
> +    Register payload = ToRegister(lir->getDef(PAYLOAD_INDEX));
> +
> +    // If the callee's global matches the current global, the implicit
> +    // |this| is always undefined.
> +    masm.ma_ldr(DTRAddr(callee, DtrOffImm(offsetof(JSObject, parent))), type, Offset);

Specifying Offset for the indexing mode isn't necessary, it is the default.

@@ +1387,5 @@
> +    // If the callee's global matches the current global, the implicit
> +    // |this| is always undefined.
> +    masm.ma_ldr(DTRAddr(callee, DtrOffImm(offsetof(JSObject, parent))), type, Offset);
> +    masm.ma_cmp(type, ImmGCPtr(gen->info().script()->global()), Assembler::Always);
> +

Likewise, specifying Assembler::Always should not be necessary, it is the default.
Comment 7 Sean Stangl [:sstangl] 2011-12-02 14:06:24 PST
Comment on attachment 578559 [details] [diff] [review]
Patch v2

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

Setting back r+ -- bugzilla dropped it with Marty's change.
Comment 8 Jan de Mooij [:jandem] 2011-12-02 14:50:51 PST
(In reply to Sean Stangl from comment #5)
> If we take an OOL stub path, will we need a postSnapshot?

Not sure, I'll look into it. Agreed that it may be a good idea to add it now, or at least a comment.

> ::: js/src/jit-test/tests/ion/callgname.js
> @@ +7,5 @@
> > +        y += g1(g1(i));
> > +    }
> > +    return y;
> > +}
> > +assertEq(f1(), 5150);
> 
> This test doesn't actually call g1 -- since g1 isn't compiled, and we can't
> yet call uncompiled functions, we'll take a bailout.

g1 is called twice inside the loop so without --ion-eager we'll compile it before we compile f1 (when f1's "use counter" is 20, g1's is ~40).

The last test fails with --ion -n so it seems useful (needs on-stack invalidation, f2 becomes a function with another global, I'll add a comment). Not sure about the second test; I'll just add the extra call to g1 though for --ion-eager.
Comment 9 Jan de Mooij [:jandem] 2011-12-07 02:20:57 PST
Created attachment 579640 [details] [diff] [review]
Follow-up patch

This required some changes after the ObjShrink merge. It now uses JSFunction::environment instead of JSObject::parent and adds MGuardClass to make sure the callee is a function. This is what JM does and it apparently works well. An advantage of MGuardClass is that we can eventually also use it also for JSOP_CALL, so that we don't have to do the "is callee a function" check twice.
Comment 10 Sean Stangl [:sstangl] 2011-12-07 14:27:02 PST
Comment on attachment 579640 [details] [diff] [review]
Follow-up patch

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

::: js/src/ion/MIR.h
@@ +1889,5 @@
> +    MGuardClass(MDefinition *obj, const Class *clasp)
> +      : MUnaryInstruction(obj),
> +        class_(clasp)
> +    {
> +        setIdempotent();

MGuardClass should indeed be idempotent, but idempotent instructions may be eliminated entirely if their result is unused, and this instruction produces no definitions.

Avoiding this requires expressing dependencies explicitly in the MIR -- it will have to actually be used by the MCall.

For the time being, unsetting the Idempotent flag from MGuardClass should be fine, since calls are effectful. If you decide to go this route, please file a follow-up bug so we remember to make the changes to MCall and place the flag back. (Otherwise, this patch would have to make the requisite changes to MCall -- and I'm eager for CALLGNAME to land.)

::: js/src/ion/shared/Assembler-shared.h
@@ +63,5 @@
>      uintptr_t value;
>  
>      explicit ImmWord(uintptr_t value) : value(value)
>      { }
> +    explicit ImmWord(const void *ptr) : value(reinterpret_cast<uintptr_t>(ptr))

Don't we want to also be able to pass in non-const addresses?

::: js/src/ion/x64/CodeGenerator-x64.cpp
@@ +422,5 @@
> +    Register tmp = ToRegister(guard->tempInt());
> +
> +    masm.loadBaseShape(obj, tmp);
> +    masm.movq(ImmWord(guard->mir()->getClass()), ScratchReg);
> +    masm.cmpq(Operand(tmp, BaseShape::offsetOfClass()), ScratchReg);

This "movq .., ScratchReg; cmpq Operand, ScratchReg" pattern is normally expressed by cmpPtr(). Could we make a new variant that takes an Operand lhs? This could also be used in visitImplicitThis below.

::: js/src/ion/x86/CodeGenerator-x86.cpp
@@ +451,5 @@
> +    Register obj = ToRegister(guard->input());
> +    Register tmp = ToRegister(guard->tempInt());
> +
> +    masm.loadBaseShape(obj, tmp);
> +    masm.cmpl(Operand(tmp, BaseShape::offsetOfClass()), ImmWord(guard->mir()->getClass()));

This is more pleasant than using loadObjClass(). Nice!
Comment 11 Jan de Mooij [:jandem] 2011-12-07 15:22:02 PST
(In reply to Sean Stangl from comment #10)
> MGuardClass should indeed be idempotent, but idempotent instructions may be
> eliminated entirely if their result is unused, and this instruction produces
> no definitions.

That's why it sets the Guard flag, it prevents removing the instruction if it has no uses. It may still be good to have explicit uses here to avoid moving the MImplicitThis/MCall before the guard. I don't think that's possible with our current GVN/LICM implementation, at least in this case, but I'm not entirely sure. Note that this also applies to MGuardShape, MBoundsCheck and probably more. I can file a follow-up bug tomorrow.

> 
> Don't we want to also be able to pass in non-const addresses?

That's still possible. The problem was that the Class is constant so we cannot pass it to a function accepting a pointer to non-constant data (since the function may change it and violate our const declaration). The opposite (non-const to const) is fine.

> 
> This "movq .., ScratchReg; cmpq Operand, ScratchReg" pattern is normally
> expressed by cmpPtr(). Could we make a new variant that takes an Operand
> lhs? This could also be used in visitImplicitThis below.

Yes, good idea.
Comment 12 Jan de Mooij [:jandem] 2011-12-08 01:45:50 PST
With cmpPtr I was able to move visitGuardShape and visitGuardClass to CodeGenerator-x86-shared. I considered moving it to CodeGenerator-shared, but we need a temp register on ARM. Combined the two patches and pushed it:

http://hg.mozilla.org/projects/ionmonkey/rev/dccc47e6137a

Note You need to log in before you can comment on or make changes to this bug.