Last Comment Bug 706986 - IonMonkey: unexpected argument types should be barriered / cause reflow
: IonMonkey: unexpected argument types should be barriered / cause reflow
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: IonOSI
  Show dependency treegraph
 
Reported: 2011-12-01 14:34 PST by Chris Leary [:cdleary] (not checking bugmail)
Modified: 2011-12-07 11:25 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reflow after bailout. (13.08 KB, patch)
2011-12-02 16:45 PST, Chris Leary [:cdleary] (not checking bugmail)
dvander: review+
Details | Diff | Splinter Review
ARM reflow. (3.05 KB, patch)
2011-12-05 14:35 PST, Chris Leary [:cdleary] (not checking bugmail)
marty.rosenberg: review+
Details | Diff | Splinter Review

Description Chris Leary [:cdleary] (not checking bugmail) 2011-12-01 14:34:06 PST
This came up when designing a test for OSI:

var causeOSI = true;

function rec(x, self) {
    if (x === 0) {
        if (causeOSI) {
            causeOSI = false;
            self(NaN, self)
            causeOSI = true;
        }
        return;
    }
    self(x - 1, self);
}

// Use enough iterations to type infer the script.
causeOSI = false;
for (var i = 0; i < 20; ++i)
    rec(1, rec);
causeOSI = true;

rec(2, rec)

We get an infer failure because the argument type mismatch causes a bailout (that thunks to the interpreter) without reflowing the type information first.

We can create a pretty simple, common thunk that reflows all the type information for all the formal arguments when any of them mismatch. When the types are *not* a mismatch type inference should just perform a single check per argument, so it doesn't seem worth the overhead to pass information on which arguments were mismatched.
Comment 1 Chris Leary [:cdleary] (not checking bugmail) 2011-12-01 14:55:11 PST
Just chatted with Brian: he says that freezing the argument types should be okay in terms of triggering recompilation (instead of an IC-repatching based approach that was used in JM+TI, to prevent recompilation for compatible-but-expanding typesets).

With that in mind, we can embed type checks into the prologue and call out to a ReflowArgumentTypes thunk, passing the current script as an argument. If the script is invalidated by the type reflow then the thunk will return an error code, causing the ion code to take a Bailout.

In JM+TI this type-checking code is a separate entry point, so that callers which know definitively that their actual arguments are compatible with the typesets of the callee's formal arguments can bypass these checks.
Comment 2 David Anderson [:dvander] 2011-12-01 15:08:40 PST
Ion scripts have the separate entry-point as well, but the type checks are inlined. If they fail we take a bailout. The problem is there is no bailout mechanism to inform of argument type-check failure. Worse, invalidation inside a bailout is unsafe, because there is no exit frame.

The call to ReflowArgumentTypes, instead of performing a bailout, would solve both these problems, as long as invalidation was guaranteed (if not we'd need a bailout after). But this is also an existing problem for type barriers. An easy fix would be to create an exit frame for ion::Bailout, but that might mess up the return path when invalidation occurs. Another idea is to return some TI data from ion::Bailout, and in the bailout assembly, create an exit frame and call to a TI handler.
Comment 3 Chris Leary [:cdleary] (not checking bugmail) 2011-12-01 17:39:30 PST
Talked with David and I'm going to try the VM-call-to-TI-reflow-after-exit-frame-has-been-created route. A different bailout status code will be returned in the case where reflow is required.

The Bailout call will be extended to return some algebraic thing like TypeBarrier(pcOffset) | ReflowArgs | Error. Handwaving a bit on that part for now, since I assume there's a way to make it happen. :-)
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2011-12-02 16:45:03 PST
Created attachment 578771 [details] [diff] [review]
Reflow after bailout.

This appears to work. Of course, without invalidation we currently fail this due to "too much recursion" because of thunking to the interpreter that re-enters the ion code that thunks to the interpreter...
Comment 5 David Anderson [:dvander] 2011-12-02 16:50:20 PST
Comment on attachment 578771 [details] [diff] [review]
Reflow after bailout.

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

Nice - a few nits, but one non-nit: Trampoline-x64 and Trampoline-arm need to be modified too.

::: js/src/ion/Bailouts.h
@@ +134,5 @@
>      Bailout_Normal,
>  
> +    // A bailout at the very start of a function indicates that there may be
> +    // a type mismatch in the arguments that necessitates a reflow.
> +    Bailout_Entry,

Bailout_ArgumentCheck or something would be clearer.

@@ +152,4 @@
>  static const uint32 BAILOUT_RETURN_OK = 0;
>  static const uint32 BAILOUT_RETURN_FATAL_ERROR = 1;
> +static const uint32 BAILOUT_RETURN_ENTRY = 2;
> +static const uint32 BAILOUT_RETURN_TYPE_BARRIER = 3;

Same.

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +368,5 @@
> +    // - 0x1: error (handle exception)
> +    // - 0x2: reflow args
> +    // - 0x3: reflow barrier
> +
> +    masm.cmpl(eax, Imm32(0x1));

Use the BAILOUT_RETURN_BLAH here?
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2011-12-05 14:35:51 PST
Created attachment 579161 [details] [diff] [review]
ARM reflow.

The patch for x64 is the same, s/eax/rax, but the ARM one is sufficiently different to warrant a review. :-)
Comment 7 Marty Rosenberg [:mjrosenb] 2011-12-06 12:17:12 PST
Comment on attachment 579161 [details] [diff] [review]
ARM reflow.

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

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +410,5 @@
> +    // - 0x2: reflow args
> +    // - 0x3: reflow barrier
> +
> +    masm.ma_cmp(r0, Imm32(BAILOUT_RETURN_FATAL_ERROR));
> +    masm.as_b(&interpret, Assembler::LessThan);

Could you use ma_b rather than as_b?
ma_* is from the macro assembler, whereas as_* is from the assembler, and in general, harder to use correctly.  My plan is to mark everything in the assembler as protected so *only* the macro assembler can use it.
Comment 8 Chris Leary [:cdleary] (not checking bugmail) 2011-12-06 16:04:28 PST
https://hg.mozilla.org/projects/ionmonkey/rev/90e4885eada0
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-12-07 11:25:55 PST
The test is known failing until we have OSI:

https://hg.mozilla.org/projects/ionmonkey/rev/188a24073bf9

Note You need to log in before you can comment on or make changes to this bug.