Closed Bug 921120 Opened 6 years ago Closed 6 years ago

IonMonkey: Enable compilation of JSOP_SETARG in the presence of hasArguments()

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: djvj, Assigned: djvj)

References

Details

Attachments

(1 file, 1 obsolete file)

Found this while looking at improving Dromaeo scores on the DOM Traversal (prototype) benchmark.  Prototype's |$| function has a JSOP_SETARG in the presence of hasArguments(), which we don't compile yet.

Problem description:

Currently we don't Ion-compile functions with JSOP_SETARG when that function also has magic arguments usage.  This is because the JSOP_GETELEM in |arguments[...]| expressions can alias:

function foo(a) {
  a = 9;
  return arguments[0];
}

In this case, the |arguments[0]| will retrieve the arguments from the frame, so |a = 9| must additionally set the frame argument (in addition to replacing the local slot definition with the new value).


The following improvements can be made:

1. In cases where we know that |arguments| does not alias formals (e.g. strict functions), we can treat the JSOP_SETARG as a simple local slot set, since that's the only way it'll be observed.

2. If we know that the contents of |arguments| are not observed (either via a |arguments[i]| or a |fun.apply(..., arguments)|, then we can also treat the JSOP_SETARG as a simple local slot set.

3. If the above two cases don't apply, we can still compile the JSOP_SETARG by emitting code to update the argument kept in the stack frame.  (This only applies to non-inlined functions, since inlined functions don't have their own stack frame entry with an arguments area).
Attached patch enable-jsop-setarg.patch (obsolete) — Splinter Review
Enable JSOP_SETARG in functions which use magic arguments.  Comment 0 describes the patch pretty well.
Attachment #810785 - Flags: review?(nicolas.b.pierron)
Note: his makes for about a 5% improvement on Dromaeo DOM Traversal (prototype).
Comment on attachment 810785 [details] [diff] [review]
enable-jsop-setarg.patch

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

::: js/src/jit/IonBuilder.cpp
@@ +1389,3 @@
>          return true;
>  
>        case JSOP_SETARG:

Create a new jsop_setarg for this case.

@@ +1407,5 @@
> +        // Otherwise, if a magic arguments is in use, and it aliases formals, and there exist
> +        // arguments[...] GETELEM expressions in the script, then SetFrameArgument must be used.
> +        // If no arguments[...] GETELEM expressions are in the script, and an argsobj is not
> +        // required, then it means that any aliased argument set can never be observed, and
> +        // the frame does not actually need to be updated with the new arg value.

Sadly no, because of “f.arguments”.  Even if a script have no arguments[…] getters, this does not mean that the argument object cannot be observed.

@@ +1416,5 @@
> +
> +            int32_t arg = GET_SLOTNO(pc);
> +            MSetFrameArgument *store = MSetFrameArgument::New(arg, current->pop());
> +            current->add(store);
> +            current->push(store);

Do not replace the original value, the fact of storing the value in the argument object does not alter the value:

   MSetFrameArgument *store = MSetFrameArgument::New(arg, current->peek(-1));
   current->add(store);

::: js/src/jit/LIR-Common.h
@@ +4545,5 @@
> +{
> +  public:
> +    LIR_HEADER(SetFrameArgumentT)
> +
> +    const LDefinition *output() {

Why a setter would have an output, which is not even used?

::: js/src/jit/MIR.h
@@ +7774,5 @@
>      }
>      AliasSet getAliasSet() const {
> +        // If the script doesn't have any JSOP_SETARG ops, then this instruction is never
> +        // aliased.
> +        if (scriptHasSetArg_)

As this is the top-level script, we can read this flag from the graph's info instead of duplicating the flag on MGetArgument.  This way you do not have to change the constructor and all call-sites.

::: js/src/jsanalyze.cpp
@@ +1574,5 @@
>  
>      /* We can read the frame's arguments directly for f.apply(x, arguments). */
>      if (op == JSOP_FUNAPPLY && GET_ARGC(pc) == 2 && use->u.which == 0) {
>          argumentsContentsObserved_ = true;
> +        script_->setArgumentsContentsObserved(true);

nit: remove SccriptAnalysis::argumentsContentObserved_ as you replaced it by this flag stored on the script.
Attachment #810785 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> @@ +1407,5 @@
> > +        // Otherwise, if a magic arguments is in use, and it aliases formals, and there exist
> > +        // arguments[...] GETELEM expressions in the script, then SetFrameArgument must be used.
> > +        // If no arguments[...] GETELEM expressions are in the script, and an argsobj is not
> > +        // required, then it means that any aliased argument set can never be observed, and
> > +        // the frame does not actually need to be updated with the new arg value.
> 
> Sadly no, because of “f.arguments”.  Even if a script have no arguments[…]
> getters, this does not mean that the argument object cannot be observed.

Good catch, I totally forgot about this.

> As this is the top-level script, we can read this flag from the graph's info
> instead of duplicating the flag on MGetArgument.  This way you do not have
> to change the constructor and all call-sites.

The MDefinition doesn't have access to the MIRGraph, so I think it still needs to be passed in.
Also, the constructor is only called in one place, so it doesn't seem like a big deal.


Other comments addressed, passing jit-tests.  Will post for re-review.  Running jit-tests now.
Addressed comments.  Also, renamed MGetArgument to MGetFrameArgument so that it's more descriptive and consistent with MSetFrameArgument.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=ca1ae44a2b45
Attachment #810785 - Attachment is obsolete: true
Attachment #811308 - Flags: review?(nicolas.b.pierron)
(In reply to Kannan Vijayan [:djvj] from comment #4)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > As this is the top-level script, we can read this flag from the graph's info
> > instead of duplicating the flag on MGetArgument.  This way you do not have
> > to change the constructor and all call-sites.
> 
> The MDefinition doesn't have access to the MIRGraph, so I think it still
> needs to be passed in.

mir->block()->graph() ?
Comment on attachment 811308 [details] [diff] [review]
enable-jsop-setarg-try2.patch

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

Fix the return type and r=me.

::: js/src/jit/IonBuilder.h
@@ +454,1 @@
>      bool jsop_funcall(uint32_t argc);

nit: Move setarg before defvar, as notearg is dealing with arguments of callees, while setarg is dealing with the argument of the current frame (closer to defvar & deffun than notearg & funcall).

::: js/src/jit/Lowering.cpp
@@ +2918,5 @@
> +{
> +    MDefinition *input = ins->input();
> +
> +    JS_ASSERT(input->type() == ins->type());
> +    JS_ASSERT(input->type() != MIRType_None);

If there is no return value, I would expect ins->type() to be MIRType_None.

::: js/src/jit/MIR.h
@@ +7791,5 @@
> +      : MUnaryInstruction(value),
> +        argno_(argno)
> +    {
> +        setResultType(value->type());
> +        setResultTypeSet(value->resultTypeSet());

MSetFrameArgument has no return value, so no return type either.
Attachment #811308 - Flags: review?(nicolas.b.pierron) → review+
Assignee: nobody → kvijayan
https://hg.mozilla.org/mozilla-central/rev/2963a336e7ec
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 925470
Fixed an uninitialized member variable, caught by valgrind:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5badb4022e83
Depends on: 942604
Depends on: 943366
Depends on: 951528
Depends on: 951970
Depends on: 944094
You need to log in before you can comment on or make changes to this bug.