Closed Bug 918405 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: djvj, Assigned: djvj)

References

Details

Attachments

(1 file)

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.
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.
Attachment #807305 - Flags: review?(hv1989)
Passes jit-tests with --ion-eager on x64 and x86, running through try here: https://tbpl.mozilla.org/?tree=Try&rev=761deb37f246
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?
Attachment #807305 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/cd646a300ffe
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Assignee: general → kvijayan
Depends on: 921035
You need to log in before you can comment on or make changes to this bug.