700111 - IonMonkey: OSR fails due to aggressive speculation

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

For IonMonkey bailouts to work, we require that the values passed around within JITted code are of types expected by TI. We enforce these types by inserting barriers at various points during which types can change, and we do not internally change types within the JITted code.

Our engine happens to know that Values of type Boolean can be unboxed to Values of type Int32. This breaks the above invariant -- if we unbox a Boolean to an Int32 and then take a bailout to an MResumePoint after the unbox, it is not guaranteed that TI will expect an Int32, and so we may trip TypeSet asserts.

OSR happens to trigger this for a bunch of tests, since the insertion of OsrPhi instructions coerces all Booleans to Values, which causes the TypeAnalysis phase to insert Unbox instructions nearby. If the point of failure is in a successor block, then the bailout is taken with the knowledge that the Boolean is really an Int32, and we die.

For the sake of example, consider the following code:

> function f(x) {
>   var c = true;
>   for (var i = 0; i < 40; i++) {
>     if (c > 1) x();
>   }
> }

We don't currently optimize away the constant expression |c > 1|.

With --ion-eager, the relevant instruction flow is:
> (0 Constant true) -> (1 MResumePoint) -> (2 ToInt32 0) -> Bail

With --ion -n and OSR, the relevant flow is:
> (0 Constant true) -> (1 OsrValue 0) -> (2 UnboxInt32 1) -> (3 MResumePoint) -> Bail

The latter flow provides an Int32(1) during bailout, which crashes. The former case optimizes away the ToInt32 during lowering, and so just remembers that the Value is of type Boolean throughout.

Comment 1

[Sean Stangl [:sstangl]]

Err, that example wasn't explained clearly -- it happens to bail because LICM will hoist the UnboxObject of "x" to the loop preheader. If f() is called with a non-Object argument, the flow will be as described.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

So, I think this case is actually okay. What we've hoisted is the MUnbox instruction, which is trying to unbox an integer, but it will fail. The bailout is on the unbox, not the call.

[Snapshots] Writing pc offset 13, nslots 4
[Snapshots]     slot 0: value (t=124, d=120)
[Snapshots]     slot 1: value (t=ebp, d=ebx)
[Snapshots]     slot 2: value (t=edi, d=esi)
[Snapshots]     slot 3: value (t=ecx, d=eax)
[Snapshots] ending snapshot total size: 22 bytes (start 42)
...
[Snapshots] Took bailout! Snapshot offset: 42

pc 13 is the JSOP_TRACE - so the loop header. Variable "c" is slot 2 which is a Value, not an int32.

There's a separate issue here, which is that we're failing to OSR due to the comparison specialization being too aggressive. That is worth keeping the bug open for.

Comment 3

[Sean Stangl [:sstangl]]

(In reply to David Anderson [:dvander] from comment #2)
> There's a separate issue here, which is that we're failing to OSR due to the
> comparison specialization being too aggressive. That is worth keeping the
> bug open for.

Rather, we are boxing the Boolean in Block 0, when it should remain a Boolean. The bug is that an MUnbox to Boolean does not exist in the OSR block, since TypeAnalysis thinks that it is valid for a Value from the OSR block to flow to the MPhi.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

This no longer bails out with --ion -n.