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).

JM+TI just optimizes arguments[i] accesses when no arguments to the function are explicitly written to.

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().

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.

This patch only add JSOP_ARGUMENTS, we should not expect any performance improvement yet, since this patch increase the compilation time with no benefits.

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 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)

[x64] Add support for lazy arguments getter.

[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 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 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 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 :(

(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 on attachment 627448 [details] [diff] [review] (part 1) Add JSOP_ARGUMENTS. https://hg.mozilla.org/projects/ionmonkey/rev/5762715ce958

Comment on attachment 629045 [details] [diff] [review] (part 2) Add Implementation of arguments.length https://hg.mozilla.org/projects/ionmonkey/rev/f4352fe7b2fb

Comment on attachment 630612 [details] [diff] [review] (part 3) Add support for arguments getter. https://hg.mozilla.org/projects/ionmonkey/rev/b02a7b214e49

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.

(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.

Is there more to do here?

(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 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.