918405 - IonMonkey: Enable OSR-ing into Ion in functions with needsArgsObj

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

This was an line-item in bug 860145 that wasn't completed since it was a less of a priority.  Now that I'm back to optimizing ArgumentsObject stuff, it seems appropriate to close off this unhandled corner case.

Recap of problem:
Ion doesn't support OSR-ing into functions which need a full ArgumentsObject.  There are some places in dromaeo jQuery 1.6.4 where we don't OSR into Ion because of this.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: enable-argumentsobj-osr.patch

Enables OSR-ing into Ion for functions which use heavyweight ArgumentsObject.

Main changes:
1. Load ArgumentsObj from the StackFrame into the OSR slot in the IonBuilder osr preheader initialization.
2. Initialize args slots (which are not aliased) from the arguments object instead of the frame.
3. Relax various asserts that prevent this from working.

Comment 2

[Kannan Vijayan [:djvj]]

Passes jit-tests with --ion-eager on x64 and x86, running through try here: https://tbpl.mozilla.org/?tree=Try&rev=761deb37f246

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 807305 [details] [diff] [review]
enable-argumentsobj-osr.patch

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

Nice!

::: js/src/jit/BaselineFrame.h
@@ -157,5 @@
>      }
>  
>      Value &unaliasedFormal(unsigned i, MaybeCheckAliasing checkAliasing = CHECK_ALIASING) const {
>          JS_ASSERT(i < numFormalArgs());
> -        JS_ASSERT_IF(checkAliasing, !script()->argsObjAliasesFormals());

I'm very confused by this removal. StackFrame::unaliasedFormal, StackFrame::unaliasedActual, BaselineFrame::unaliasedActual also has this assert. So seems wrong to delete.

I think in the current code we do
unaliasedFormal(i, DONT_CHECK_ALIASING); 
in that case.

So for consistency we should do that too.

::: js/src/jit/BaselineIC.cpp
@@ +846,5 @@
>  
>      // Copy formal args and thisv.
>      memcpy(stackFrameStart, frame->argv() - 1, (numFormalArgs + 1) * sizeof(Value));
>  
>      // Initialize ScopeChain, Exec, and Flags fields in StackFrame struct.

Update comment

::: js/src/jit/IonBuilder.cpp
@@ +5762,5 @@
> +                // If this is an aliased formal, then the arguments object contains a hole
> +                // at this index.  Any references to this variable in the jitcode will come
> +                // from JSOP_*ALIASEDVAR opcodes, so the slot itself can be set to
> +                // undefined.  If it's not aliased, it must be retrieved from the arguments
> +                // object.

Length of comments are 80 characters. I think this doesn't fit ...

@@ +5829,5 @@
>      // at the loop header are known.
>      for (uint32_t i = info().startArgSlot(); i < osrBlock->stackDepth(); i++) {
>          MDefinition *existing = current->getSlot(i);
>          MDefinition *def = osrBlock->getSlot(i);
> +        JS_ASSERT_IF(!def->isConstant(), def->type() == MIRType_Value);

That is the Undefined value flowing in right.
I would prefer:
JS_ASSERT_IF(!needsArgObj || !info().isSlotAliased(i), def->type() == MIRType_Value);

::: js/src/jit/MIRGraph.cpp
@@ +463,3 @@
>          } else {
> +            JS_ASSERT(def->isOsrValue() || def->isGetArgumentsObjectArg() || def->isConstant());
> +            JS_ASSERT_IF(def->isConstant(), def->toConstant()->value() == UndefinedValue());

Can you add a comment when we have undefined, i.e. argsObject that is aliased? Here and above?

Comment 4

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd646a300ffe

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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