Last Comment Bug 722331 - IonMonkey: Bailing out when unboxing OSR values
: IonMonkey: Bailing out when unboxing OSR values
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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 Brian Hackett (:bhackett) 2012-01-30 11:20:42 PST
Sure, 2) should work fine.
Comment 2 Jan de Mooij [:jandem] 2012-01-30 14:13:47 PST
Created attachment 592856 [details] [diff] [review]
Patch

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 Brian Hackett (:bhackett) 2012-01-30 15:57:24 PST
Comment on attachment 592856 [details] [diff] [review]
Patch

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 Jan de Mooij [:jandem] 2012-01-31 00:33:23 PST
https://hg.mozilla.org/projects/ionmonkey/rev/990ee31c370a

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.