Last Comment Bug 735406 - IonMonkey: Handle JSOP_ARGUMENTS
: IonMonkey: Handle JSOP_ARGUMENTS
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
:
Mentors:
Depends on: 733950 759442 776687 899402 930437
Blocks: IonSpeed 758307 771500 735402
  Show dependency treegraph
 
Reported: 2012-03-13 13:03 PDT by David Anderson [:dvander]
Modified: 2013-10-25 03:55 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(part 1) Add JSOP_ARGUMENTS. (10.43 KB, patch)
2012-05-26 00:21 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
nicolas.b.pierron: checkin+
Details | Diff | Splinter Review
(part 2) Add Implementation of arguments.length (19.23 KB, patch)
2012-05-31 21:30 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
nicolas.b.pierron: checkin+
Details | Diff | Splinter Review
(part 3) Add support for arguments getter. (17.84 KB, patch)
2012-06-06 10:16 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
nicolas.b.pierron: checkin+
Details | Diff | Splinter Review
(part 4) Add support for arguments setter. (11.82 KB, patch)
2012-06-06 10:33 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
nicolas.b.pierron: checkin-
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2012-03-13 13:03:09 PDT
We need to support JSOP_ARGUMENTS for earley-boyer, and probably other benchmarks too. However, we don't want to support the whole thing just yet, only the cases where we don't actually need to reify an arguments object. That means:

 (1) If we need to reify argsobj, we should bailout.
 (2) We cannot enter a function, or a loop, if the frame has an argsobj.

The use case we are trying to optimize is from earley-boyer:

> function sc_list() {
>     var res = null;
>     var a = arguments;
>     for (var i = a.length-1; i >= 0; i--)
>         res = new sc_Pair(a[i], res);
>     return res;
> }

Here, ideally, we want transform a[i] into a read off the stack. Note that we have to be careful, if there is a formal argument present that becomes overwritten, that has to be reflected through the arguments access (or maybe argument writes should disable this optimization). Note also that we have to be careful with inlining - we cannot inline this function (because we wouldn't have a coherent arguments vector).
Comment 1 Brian Hackett (:bhackett) 2012-03-13 13:05:34 PDT
JM+TI just optimizes arguments[i] accesses when no arguments to the function are explicitly written to.
Comment 2 Luke Wagner [:luke] 2012-03-13 13:59:12 PDT
Yeah, zero arguments objects are created in v8.  Once bug 731868 gets sorted out and I can land bug 733950, you can specialize at jit-compile time on script->needsArgsObj().
Comment 3 Nicolas B. Pierron [:nbp] 2012-05-24 11:10:00 PDT
To make it simple first, I will open a follow-up bug for optimizing arguments.length. In the mean time I plan to implement arguments.length as a VM call which initialize an IonFrameIterator and call numActualArgs.  This would be extremely slow, but arguments.length is pure (In IonMonkey) since we never manipulate an argument object.  Thus even if it is slow, it can be optimized by LICM and GVN.
Comment 4 Nicolas B. Pierron [:nbp] 2012-05-26 00:21:53 PDT
Created attachment 627448 [details] [diff] [review]
(part 1) Add JSOP_ARGUMENTS.

This patch only add JSOP_ARGUMENTS, we should not expect any performance improvement yet, since this patch increase the compilation time with no benefits.
Comment 5 Nicolas B. Pierron [:nbp] 2012-05-31 21:30:32 PDT
Created attachment 629045 [details] [diff] [review]
(part 2) Add Implementation of arguments.length

Handle arguments.length.

- fix some unboxing issues.
- handle Ion frames in js_fun_apply (should be removed with the optim of js_fun_apply in Ion)
- Add argument policy.
- Based on Bug 759442 patches.
Comment 6 David Anderson [:dvander] 2012-06-05 18:27:30 PDT
Comment on attachment 629045 [details] [diff] [review]
(part 2) Add Implementation of arguments.length

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

Nice! r=me with comments addressed

::: js/src/ion/TypePolicy.cpp
@@ +355,5 @@
> +    in = boxAt(ins, in);
> +    MUnbox *replace = MUnbox::New(in, MIRType_Magic, MUnbox::Fallible);
> +    ins->block()->insertBefore(ins, replace);
> +    ins->replaceOperand(Op, replace);
> +    return true;

Do we actually need this policy? The input is never read, and the opcode is only generated if TI guaranteed it's safe. Seems like we can get rid of it. That means the |JS_ASSERT(type == MIRType_Magic)| assert in Lowering.cpp can go too since it just checks the type policy. We can even drop the operand/input to ArgumentsLength as well.

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +594,5 @@
> +        unboxInt32(src, dest);
> +    }
> +    void unboxMagic(const Address &src, const Register &dest) {
> +        unboxMagic(Operand(src), dest);
> +    }

Are these unboxMagics used for anything yet? (It seems like they don't exist on other archs)
Comment 7 Nicolas B. Pierron [:nbp] 2012-06-06 10:16:44 PDT
Created attachment 630612 [details] [diff] [review]
(part 3) Add support for arguments getter.

[x64] Add support for lazy arguments getter.
Comment 8 Nicolas B. Pierron [:nbp] 2012-06-06 10:33:00 PDT
Created attachment 630619 [details] [diff] [review]
(part 4) Add support for arguments setter.

[x64] Add support for argument setter.

Currently the patch is erroneous because it does not restore arguments modifications in the formal arguments.  This is critical because the formal arguments indexed in the snapshots are used for bailouts.  One way would be to force the LAllocation of the formal arguments if there is any argument setter — either setArg or arguments_setelem

This is a real problem some benchmark functions (cf earley boyer) are using a formal argument name and are reading/writing arguments with the formal argument and the lazy set of arguments:

function sc_map(proc, l1) {
    if (l1 === undefined)
        return null;
    // else
    var nbApplyArgs = arguments.length - 1;
    var applyArgs = new Array(nbApplyArgs);
    var revres = null;
    while (l1 !== null) {
        for (var i = 0; i < nbApplyArgs; i++) {
            applyArgs[i] = arguments[i + 1].car;
            arguments[i + 1] = arguments[i + 1].cdr;
        }
        revres = sc_cons(proc.apply(null, applyArgs), revres);
    }
    return sc_reverseAppendBang(revres, null);
}
Comment 9 David Anderson [:dvander] 2012-06-14 17:46:10 PDT
Comment on attachment 630612 [details] [diff] [review]
(part 3) Add support for arguments getter.

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

::: js/src/ion/AliasAnalysis.cpp
@@ +149,5 @@
>                  }
>  
> +                if (!lastStore) {
> +                    IonSpew(IonSpew_Alias, "Load %d depends on nothing", def->id());
> +                    continue;

Jan should review this (maybe just ping him on IRC) since I'm not familiar with the AA code - it looks like the |continue| changes some functionality though.

::: js/src/ion/CodeGenerator.cpp
@@ +2055,5 @@
> +    const LAllocation *index = lir->index();
> +    size_t argvOffset = frameSize() + IonJSFrameLayout::offsetOfActualArgs();
> +
> +    if (index->isConstant()) {
> +        uint8 i = index->toConstant()->toInt32();

guaranteed uint8?

::: js/src/ion/IonBuilder.cpp
@@ +4358,4 @@
>      MDefinition *idx = current->pop();
> +    MDefinition *args = current->pop();
> +
> +    if (idx->isConstant()) {

Does this case appear in the benchmarks? If not, I'd consider removing it, or extracting the first few lines of the logic into a helper function. (like lines 4361-4367).

@@ +4384,5 @@
> +    MBoundsCheck *check = MBoundsCheck::New(index, length);
> +    current->add(check);
> +
> +    // Load the argument from the actual arguments.
> +    MArgumentsGet *load = MArgumentsGet::New(index);

nit: This (and the LIR equivalent) should be named "GetArgument" to match GetElement, GetProperty, etc.

::: js/src/ion/MIR.h
@@ +195,5 @@
>          ObjectFields      = 1 << 0, // shape, class, slots, length etc.
>          Element           = 1 << 1, // A member of obj->elements.
>          Slot              = 1 << 2, // A member of obj->slots.
>          TypedArrayElement = 1 << 3, // A typed array element.
> +        ActualArgs        = 1 << 4, // .

nit: .
Comment 10 David Anderson [:dvander] 2012-06-14 17:49:03 PDT
Comment on attachment 630619 [details] [diff] [review]
(part 4) Add support for arguments setter.

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

Nice, glad this was pretty straightforward.

::: js/src/ion/Lowering.cpp
@@ +1576,5 @@
> +    if (!useBox(lir, LArgumentsSet::Value, ins->value()))
> +        return false;
> +    if (!add(lir, ins))
> +        return false;
> +    return assignSafepoint(lir, ins);

Is a safepoint actually needed here?

::: js/src/ion/MOpcodes.h
@@ +146,5 @@
>      _(IteratorEnd)                                                          \
>      _(StringLength)                                                         \
>      _(ArgumentsLength)                                                      \
>      _(ArgumentsGet)                                                         \
> +    _(ArgumentsSet)                                                         \

nit: s/ArgumentsSet/SetArgument/
Comment 11 David Anderson [:dvander] 2012-06-14 17:51:22 PDT
Comment on attachment 630612 [details] [diff] [review]
(part 3) Add support for arguments getter.

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

::: js/src/ion/IonBuilder.cpp
@@ +4367,5 @@
> +            uint32 index = idxv.toInt32();
> +            if (index < info().nargs()) {
> +                current->pushArg(index);
> +                return true;
> +            }

Actually we have to be super careful: imagine something like

for (var i = 0; i < 4; i++) {
    arguments[i] = 5;
    print(arguments[3]);
}

Since the slots are still aliased in some regard it might not be possible to use SSA :(
Comment 12 Nicolas B. Pierron [:nbp] 2012-06-15 07:10:55 PDT
(In reply to David Anderson [:dvander] from comment #10)
> Comment on attachment 630619 [details] [diff] [review]
> (part 4) Add support for arguments setter.
> 
> Review of attachment 630619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, glad this was pretty straightforward.
> 
> ::: js/src/ion/Lowering.cpp
> @@ +1576,5 @@
> > +    if (!useBox(lir, LArgumentsSet::Value, ins->value()))
> > +        return false;
> > +    if (!add(lir, ins))
> > +        return false;
> > +    return assignSafepoint(lir, ins);
> 
> Is a safepoint actually needed here?

My mistake at copying the wrong code. A snapshot is needed but not a safepoint.

> ::: js/src/ion/IonBuilder.cpp
> @@ +4367,5 @@
> > +            uint32 index = idxv.toInt32();
> > +            if (index < info().nargs()) {
> > +                current->pushArg(index);
> > +                return true;
> > +            }
> 
> Actually we have to be super careful: imagine something like
> 
> for (var i = 0; i < 4; i++) {
>     arguments[i] = 5;
>     print(arguments[3]);
> }
> 
> Since the slots are still aliased in some regard it might not be possible to
> use SSA :(

In the presence of potential formal argument setters, we are forced to alias the arguments to the stack space.  The best would be to insert some checks in the pre-compiled analysis to collect this information and switch all getArg / setArg to GetArgument / SetArgument to prevent aliasing issues by using the AliasAnalysis.


(In reply to David Anderson [:dvander] from comment #6)
> Comment on attachment 629045 [details] [diff] [review]
> (part 2) Add Implementation of arguments.length
> 
> Review of attachment 629045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! r=me with comments addressed
> 
> ::: js/src/ion/TypePolicy.cpp
> @@ +355,5 @@
> > +    in = boxAt(ins, in);
> > +    MUnbox *replace = MUnbox::New(in, MIRType_Magic, MUnbox::Fallible);
> > +    ins->block()->insertBefore(ins, replace);
> > +    ins->replaceOperand(Op, replace);
> > +    return true;
> 
> Do we actually need this policy? The input is never read, and the opcode is
> only generated if TI guaranteed it's safe. Seems like we can get rid of it.
> That means the |JS_ASSERT(type == MIRType_Magic)| assert in Lowering.cpp can
> go too since it just checks the type policy. We can even drop the
> operand/input to ArgumentsLength as well.

Currently I prefer to keep it because from this current state it does not seems hard to add support for real ArgumentObject.  Later we can optimize this if needed.

> ::: js/src/ion/x64/MacroAssembler-x64.h
> @@ +594,5 @@
> > +        unboxInt32(src, dest);
> > +    }
> > +    void unboxMagic(const Address &src, const Register &dest) {
> > +        unboxMagic(Operand(src), dest);
> > +    }
> 
> Are these unboxMagics used for anything yet? (It seems like they don't exist
> on other archs)

I replaced it by a test of the Magic value.  The unbox can be optimized out by the GVN, so I won't worry much about it for now.  And identically to my previous remark, it seems like we can handle Argument Object as v8 do.

When the magic is detected, I output a NULL as a result of the unboxing which correspond to the lazy Argument Object since we do not expect to see any ArgumentObject to be NULL.
Comment 13 Nicolas B. Pierron [:nbp] 2012-06-15 07:15:32 PDT
Comment on attachment 627448 [details] [diff] [review]
(part 1) Add JSOP_ARGUMENTS.

https://hg.mozilla.org/projects/ionmonkey/rev/5762715ce958
Comment 14 Nicolas B. Pierron [:nbp] 2012-06-15 07:32:10 PDT
Comment on attachment 629045 [details] [diff] [review]
(part 2) Add Implementation of arguments.length

https://hg.mozilla.org/projects/ionmonkey/rev/f4352fe7b2fb
Comment 15 Nicolas B. Pierron [:nbp] 2012-06-15 07:40:16 PDT
Comment on attachment 630612 [details] [diff] [review]
(part 3) Add support for arguments getter.

https://hg.mozilla.org/projects/ionmonkey/rev/b02a7b214e49
Comment 16 David Anderson [:dvander] 2012-06-15 17:33:49 PDT
Hrm, I'm trying to debug some regressions in v8 - we might need to back this out unless we can fix it in-tree. Some issues:

 - LUnbox on x86/arm has a typo, ::payload() returns getOperand(1) instead of getOperand(0)
 - It is illegal to overwrite the output inside an LUnbox because the definition is passthrough.
 - A TypeBarrier is sporadically failing inside sc_list and the register is filled with garbage
   (though sometimes it is filled with a type tag).

Taking a look at the MUnbox changes, I think it was a mistake to shoehorn MIRType_ArgsObj in. It makes interaction with bailouts, phis, and osr much more complicated. Instead we should just insert an MConstant(MagicValue(JS_OPTIMIZED_ARGUMENTS)). It looks like nothing actually uses the lazy arguments value as an actual input, so we shouldn't have to unbox or guard on it (we should not OSR into frames with an argsobj) - but worst case, we can introduce an MGuardMagic.
Comment 17 Nicolas B. Pierron [:nbp] 2012-06-15 22:46:30 PDT
(In reply to David Anderson [:dvander] from comment #16)
> Hrm, I'm trying to debug some regressions in v8 - we might need to back this
> out unless we can fix it in-tree. Some issues:
> 
>  - LUnbox on x86/arm has a typo, ::payload() returns getOperand(1) instead
> of getOperand(0)

https://hg.mozilla.org/projects/ionmonkey/rev/17db7530ad47

>  - It is illegal to overwrite the output inside an LUnbox because the
> definition is passthrough.

https://hg.mozilla.org/projects/ionmonkey/rev/1c9c3fa92fa8

>  - A TypeBarrier is sporadically failing inside sc_list and the register is
> filled with garbage
>    (though sometimes it is filled with a type tag).

https://hg.mozilla.org/projects/ionmonkey/rev/c13992e8edbd

And the test failure reported by marty on IRC, cause by an off-by-one read is in fact a wrong argument size ( sizeof(Value*) -> sizeof(Value) ) for access fixed by

https://hg.mozilla.org/projects/ionmonkey/rev/de23a9fc29db


> Taking a look at the MUnbox changes, I think it was a mistake to shoehorn
> MIRType_ArgsObj in. It makes interaction with bailouts, phis, and osr much
> more complicated. Instead we should just insert an
> MConstant(MagicValue(JS_OPTIMIZED_ARGUMENTS)). It looks like nothing
> actually uses the lazy arguments value as an actual input, so we shouldn't
> have to unbox or guard on it (we should not OSR into frames with an argsobj)

The MIRType_ArgObj is used to work around the way snapshots are encoding the constants.  Snapshots are using the MAGIC tag with a special meaning.  The MIRType_ArgObj is used to work around that.

An other solution which might be simpler would be to hack the OSR to output the same constant when TI can predict that this is a lazy value and to replace the MPhi generation of 2 identical MConstant by an identical MConstant such as we never end-up with one put in a register.  The MConstant need to exists in the MIR.

Currently, only arguments.length depends on this unused value, and it can be removed after-all.

The sad thing about removing all these relations is that it would be harder to later add support for anykind of argument object, which might not be that difficult if needed.

> - but worst case, we can introduce an MGuardMagic.

True, the only detail is that the code assume that everything can be unbox and it sounds just easier to put this stuff in the unbox than adding conditions to avoid generating the unbox in such cases.
Comment 18 Paul Wright 2012-07-03 12:15:11 PDT
Is there more to do here?
Comment 19 Nicolas B. Pierron [:nbp] 2012-07-04 02:25:52 PDT
(In reply to Paul Wright from comment #18)
> Is there more to do here?

Part 4 is not landed yet, but it is debatable if it should land or not.  What we can do is close this bug and create a follow-up bug for the setter.
Comment 20 Nicolas B. Pierron [:nbp] 2012-07-06 06:49:41 PDT
Comment on attachment 630619 [details] [diff] [review]
(part 4) Add support for arguments setter.

This attachment as been moved to Bug 771500 which deals with argument setters.

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