IonMonkey: Bailing out when unboxing OSR values




JavaScript Engine
6 years ago
6 years ago


(Reporter: jandem, Assigned: jandem)


(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)



(1 attachment)



6 years ago
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.
Sure, 2) should work fine.

Comment 2

6 years ago
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.
Attachment #592856 - Flags: review?(dvander)
Attachment #592856 - Flags: review?(bhackett1024)
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.
Attachment #592856 - Flags: review?(bhackett1024) → review+
Attachment #592856 - Flags: review?(dvander) → review+

Comment 4

6 years ago

I'll file a follow-up bug for the LICM issue in comment 3.
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.