We currently optimize arguments by building MIR for each script that uses arguments before baseline compilation, determining whether the arguments object escapes, and (if not) passing around a magic value to represent the uninstantiated arguments object. This has a few downsides. It means we have to build MIR much earlier than we otherwise would, which slows down baseline execution. It's also quite complicated to make sure that the magic value doesn't leak out to code where it doesn't belong (and somewhat error-prone).

An alternative implementation is to treat this as just another example of scalar replacement, similar to what we already do with objects and arrays. This would only apply in Warp, but hopefully the cost of allocating the arguments object more often in baseline would be outweighed by the benefit of not having to build MIR to do the analysis.

Looking at a try run with the arguments analysis disabled, it appears that the EmberJS benchmarks in Speedometer and the Box2D and RayTrace benchmarks in Octane get the most value from the existing optimization.

Adding a jit option so that I can prototype this behind a flag.

The goal of scalar replacement of arguments is to remove all uses of the CreateArgumentsObject instruction, so that DCE can remove it.

However, the MCreateArgumentsObject constructor calls setGuard, which stops DCE from removing it. (This has been true since it was introduced.) As far as I can tell, it doesn't have to be a guard. In FinishBailoutToBaseline, if a frame needs an args object and doesn't have one, then we call ArgumentsObject::createExpected to create it from the restored baseline frame.

Depends on D103108

This is based on the existing code for scalar replacement of objects and arrays. Unlike objects and arrays, we don't have to support mutation of the arguments object, so we can simplify a lot of the code: for example, we don't need to track state per-block.

This initial patch adds support for arguments.length.

Depends on D103109

This adds support for JSOp::GetArg when arguments aliases the formal args.

Depends on D103110

This adds support for arguments[i]. It is based directly on WarpCacheIRTranspiler::emitLoadFrameArgumentResult.

If this bounds check fails, then baseline will attach a new stub in tryAttachGenericElement, preventing a bailout loop. We could also disable scalar replacement of arguments immediately, using a special BailoutKind.

Depends on D103111

Backed out for causing build bustages on ScalarReplacement.cpp.

Backout link: https://hg.mozilla.org/integration/autoland/rev/a5f845544279d4b48339bac6d2bf86ce54c0696e

Push with failures:https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=OBVutWyTQP28MZHTI9Jzow.0&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Cusercancel%2Crunnable&revision=ee7b88395348b5850a5340610cd525201b5fcadb&searchStr=build

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328512693&repo=autoland&lineNumber=16893

This confused me for a moment. It turns out that the problem was that I hadn't rebased on the latest copy of m-c, and although there were no merge conflicts, recent changes to m-c meant that the part of my patch adding isArgumentsObjectClass to MGuardToClass was incorrectly applied to MHasClass. I've rebased the patch. I'll give it a while to give reviewbot a chance to warn me if it's still a problem, and then I'll reland.

https://hg.mozilla.org/mozilla-central/rev/a85f89802dcf
https://hg.mozilla.org/mozilla-central/rev/ad86d5f1d5c7
https://hg.mozilla.org/mozilla-central/rev/c8bef0799361
https://hg.mozilla.org/mozilla-central/rev/ec8cf08562a3
https://hg.mozilla.org/mozilla-central/rev/57bcdf857d44

Unlike JS_OPTIMIZED_ARGUMENTS, which is not allowed to escape its own frame, an ArgumentsObject can be passed into a different function and cause us to attach a specialized stub. In general, this all works out well. However, it does mean that some assertions in ArgumentsReplacer are too conservative: even before we support inlining functions that use arguments, it is valid to have ArgumentsObject-specific MIR that does not use the CreateArgumentsObject we are replacing.

This patch fixes those assertions and adds some test coverage.

Subsequent patches will introduce FunApplyArgsObj. I'm renaming FunApplyArgs to FunApplyMagicArgs in most places to make it easier to distinguish the two cases. (The exception is MApplyArgs/LApplyArgsGeneric, which I didn't rename because they can be produced by scalar replacement.)

Depends on D104481

The only places we check ELEMENT_OVERRIDDEN_BIT are in GetPropIRGenerator::tryAttachArgumentsObjectArg and MacroAssembler::loadArgumentsObjectElement. In both cases, we subsequently also check for deleted elements. We can remove the redundant check.

This is consistent with ArgumentsObject::obj_delProperty, which sets the other overridden bits when deleting the corresponding property.

Depends on D104482

Being able to test whether an ArgumentsObject has closed-over arguments by checking a flag instead of testing each argument makes future patches significantly simpler.

I considered just making closed-over arguments set ELEMENT_OVERRIDDEN_BIT, but this would make ArgumentsObjectArg fail if any argument was closed over, even if the specific argument we are loading is not.

Depends on D104483

Instead of adding GuardArgumentsObjectNotOverriddenElement and GuardArgumentsObjectNotForwardedArgument to go with GuardArgumentsObjectNotOverriddenIterator, I collapsed them into a single op.

This may also be useful in the future for supporting arguments.callee.

Depends on D104484

pushFunApplyArgsObj is roughly based on pushArrayArguments.

Depends on D104485

Pushing the contents of the args array in an ArgumentsData (after we have guaranteed that there are no forwarded arguments) is almost the same as pushing the contents of an elements array, but with an additional offset. In preparation for the next patch, this patch refactors PushElementsAsArguments to take an offset, and changes names/comments accordingly.

Depends on D104486

As much as possible, I based LApplyArgsObj on LApplyArrayGeneric.

I wrote the tests here by looking at existing apply testcases in jit-tests/tests/arguments and writing similar test cases where the arguments object escapes.

Depends on D104487

The original version of this patch only recovered CreateArgumentsObject on bailout if it didn't escape, but I removed that code based on discussion in #warpbuilder.

This code is tested by /ion/dce-with-rinstructions.js. Prior to this patch, it failed with --scalar-replace-arguments.

Depends on D104488

Depends on D104489

https://hg.mozilla.org/mozilla-central/rev/834df8b4b61d
https://hg.mozilla.org/mozilla-central/rev/21d7ab0b3caf
https://hg.mozilla.org/mozilla-central/rev/46a605bafc76
https://hg.mozilla.org/mozilla-central/rev/81fc994238fb
https://hg.mozilla.org/mozilla-central/rev/f177c8c6bc29
https://hg.mozilla.org/mozilla-central/rev/1d5b44c118ce
https://hg.mozilla.org/mozilla-central/rev/bf23844b260b
https://hg.mozilla.org/mozilla-central/rev/adaf366a60fa
https://hg.mozilla.org/mozilla-central/rev/375186706cae
https://hg.mozilla.org/mozilla-central/rev/776c08d542f8

This is the simplest feasible approach to codegen: arguments are passed to LCreateInlinedArgumentsObject as value operands, pushed on the stack, and passed to ArgumentsObject::create. The number of operands is constrained by register pressure on x86 (and arm32 is up against the limit with 3).

MCreateArgumentsObject has an optimization where we allocate the ArgumentsObject in jitcode and only call into C++ to fill it in. In the interest of simplicity and keeping register pressure down, I didn't try implementing that approach for MCreateInlinedArgumentsObject. Marginal improvements in performance for CreateInlinedArgumentsObject don't seem very valuable because our hope is to remove it via scalar replacement.

Depends on D106385

The commented-out line in the testcase will be uncommented in a subsequent patch once we support scalar replacement of inlined arguments and can replace a use.

Depends on D106707

recover-inline-arguments.js was added in the previous patch. We can uncomment the second half now that scalar replacement is supported.

Depends on D106708

Depends on D106709

Backed out 2 changesets (Bug 1688033) for causing mass failures in BytecodeLocation.h CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=NrlIfjTUQKmsKAjRDA5fig.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=52dd0e379bf261aaea642a0a3ad5e74037db0a4a

Failure log: https://treeherder.mozilla.org/logviewer?job_id=331672690&repo=autoland&lineNumber=5677

Backout: https://hg.mozilla.org/integration/autoland/rev/345dcf60b9a81a28f4ea9628fd9623401affc2ee

I hoisted makeCall out of WarpBuilderShared. The only changes are adding addUndefined and passing in alloc.

The pc argument to the CallInfo constructor was only used for setting the apply_ flag. Outside of assertions, the only use for this flag was in dead code in pushCallStack. (The code was originally designed to support inlining foo.apply(..., arguments). We don't currently support that, and we won't be able to support it in the future because trial inlining doesn't know where arguments comes from without escape analysis.) Instead of trying to smuggle pc into ScalarReplacement, I just removed apply_ entirely and replaced the assertions with a comment.

Depends on D106956

Depends on D106957

https://hg.mozilla.org/mozilla-central/rev/f029f97be867
https://hg.mozilla.org/mozilla-central/rev/189922696db5
https://hg.mozilla.org/mozilla-central/rev/235652e53614
https://hg.mozilla.org/mozilla-central/rev/82808be2d113
https://hg.mozilla.org/mozilla-central/rev/e57fcca626d0

Depends on D107303

If we use arguments in a loop, the transpiled GuardToObject sees a value-typed phi node and emits an MUnbox. GVN eventually cleans it up, but not before it blocks scalar replacement of arguments.

(Something similar seems to be happening in escape analysis for arrays and objects. I'm going to do a more systematic investigation of whether/how Warp broke escape analysis for objects and arrays once I'm done with arguments.)

Depends on D107304

https://hg.mozilla.org/mozilla-central/rev/43ce6ea2f963
https://hg.mozilla.org/mozilla-central/rev/2e361f7da898
https://hg.mozilla.org/mozilla-central/rev/b483df09813b

https://hg.mozilla.org/mozilla-central/rev/42f205050335
https://hg.mozilla.org/mozilla-central/rev/a6a48aa425d6
https://hg.mozilla.org/mozilla-central/rev/92752f60ee8d

Tom pointed out in #warpbuilder that SkipUninterestingInstructions unwraps MToNumberInt32, so in theory we could have a non-int32 constant. I don't think it can happen in practice, but better safe than sorry.

https://hg.mozilla.org/mozilla-central/rev/dc090a337afa

Now that the the train has left for 88, we can turn this on by default to get as much testing as possible.

Removing leave-open tag. I'll open a follow-up bug to remove the old arguments analysis code after the next release.

https://hg.mozilla.org/mozilla-central/rev/a2e2f181d820