Closed Bug 694169 Opened 13 years ago Closed 13 years ago

IonMonkey: Handle JSOP_CALLGNAME

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Handling JSOP_CALLGNAME will greatly increase test coverage.
Depends on: 680315
Blocks: 684381
Assignee: sstangl → jdemooij
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #578357 - Attachment is obsolete: true
Attachment #578542 - Flags: review?(sstangl)
Attachment #578542 - Flags: review?(sstangl)
Attached patch Patch v2Splinter Review
Add ARM support. We already have LoadSlotV on ARM, only LoadSlotT is missing.
Attachment #578542 - Attachment is obsolete: true
Attachment #578559 - Flags: review?(sstangl)
Attachment #578559 - Flags: review?(mrosenberg)
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.
Attachment #578559 - Flags: review?(sstangl) → review+
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.
Attachment #578559 - Flags: review?(sstangl)
Attachment #578559 - Flags: review?(mrosenberg)
Attachment #578559 - Flags: review+
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.
Attachment #578559 - Flags: review?(sstangl) → review+
(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.
Attached patch Follow-up patchSplinter Review
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.
Attachment #579640 - Flags: review?(sstangl)
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!
Attachment #579640 - Flags: review?(sstangl) → review+
Blocks: 708441
(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.
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.