Last Comment Bug 722331 - IonMonkey: Bailing out when unboxing OSR values
: IonMonkey: Bailing out when unboxing OSR values
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: IonSpeed
  Show dependency treegraph
Reported: 2012-01-30 08:33 PST by Jan de Mooij [:jandem]
Modified: 2012-01-31 00:33 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (5.29 KB, patch)
2012-01-30 14:13 PST, Jan de Mooij [:jandem]
dvander: review+
bhackett1024: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2012-01-30 08:33:33 PST
Consider this micro-benchmark (basic-fpops from AWFY assorted).
function test_add(x, y) {
    var z = 0;
    for (var i = 0; i < x; i++)
        z += y;
    return z;

var t = new Date;
test_add(0x3ffffff, 10.5);
print(new Date - t);
|z| is a double at the loop header, but is unboxed as int32, since it has type int32 at the predecessor.

To fix this, we can either:

1) Unbox based on TypeScript::SlotTypes. This works fine for the micro-benchmark but is not as precise as we want, since it does not operate on SSA values.

2) Use the "newValues" analysis info (a list of SSAValue's which have different types at the join point).

3) Not eagerly unboxing OSR values for which the predecessor type != the known slot type.

I'd like to try 2) first. Brian, do you think this is okay/safe?

This problem affects SS partial-sums and crypto-sha1.
Comment 1 User image Brian Hackett (:bhackett) 2012-01-30 11:20:42 PST
Sure, 2) should work fine.
Comment 2 User image Jan de Mooij [:jandem] 2012-01-30 14:13:47 PST
Created attachment 592856 [details] [diff] [review]

Passes tests. The micro-benchmark in comment 0 drops from 3100 ms to 230 ms and some SS tests are a bit faster (most notably partial-sums, 22 ms to 14 ms). Comment 0 was wrong about crypto-sha1, I think that's another problem after all.
Comment 3 User image Brian Hackett (:bhackett) 2012-01-30 15:57:24 PST
Comment on attachment 592856 [details] [diff] [review]

Review of attachment 592856 [details] [diff] [review]:

If I had to guess, since crypto-sha1 regressed with bug 719541 the problem there is probably that we hoist a bounds check badly and take bailouts because of that.  JM+TI solves this by tracking when it invalidates code due to a failed hoisted check, and after recompiling it does not do hoisting for the involved script.  IM will need something similar.
Comment 4 User image Jan de Mooij [:jandem] 2012-01-31 00:33:23 PST

I'll file a follow-up bug for the LICM issue in comment 3.

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