829277 - IonMonkey: truncate analysis can truncate prematurely.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mjrosenb/patches/fixtruncate-r0.patch

the testcase that nbp gave in bug 825006 can actually trigger errant behavior in ionmonkey as it stands.  Limit the number of instructions that truncation analysis will operate on.

Comment 1

[Marty Rosenberg [:mjrosenb]]

I just instrumented the code a bit, and it looks like it never triggers on kraken or v8, but it does trigger on the test case, so it should fix problems without causing any regressions.

Comment 2

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

Comment on attachment 700658 [details] [diff] [review]
/home/mjrosenb/patches/fixtruncate-r0.patch

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

Thanks for fixing this!

::: js/src/ion/EdgeCaseAnalysis.cpp
@@ +92,4 @@
>              continue;
> +        }
> +        if (def->isSub() && def->toSub()->isTruncated()) {
> +            ret = Max(ret, def->toSub()->isTruncated()+1);

nit: spaces around +, here and above.

::: js/src/ion/MIR.cpp
@@ +876,5 @@
>  {
> +    // the add is fallible if range analysis does not say that it is finite, AND
> +    // either the truncation analysis shows that there are non-truncated uses, or
> +    // there are more than 20 operations before it gets truncated
> +    return (!isTruncated() || isTruncated() > 20) && (!range() || !range()->isFinite());

Could you explain where 20 comes from and what it means?

Comment 3

[Marty Rosenberg [:mjrosenb]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2983ba3b514f

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2983ba3b514f

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2983ba3b514f

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2983ba3b514f