578290 - nanojit: fold constant 64-bit integer expressions

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [inactive]]

ExprFilter::ins2() folds 32-bit integer and 64-bit float expressions.  We should do likewise for 64-bit integer expressions.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

Pretty straightforward!

Comment 2

[Nicholas Nethercote [inactive]]

This can't land until bug 578292 is fixed because it will break TM, causing lots of "constantly false branch detected" assertions.

Comment 3

[Steven Johnson]

Comment on attachment 457021 [details] [diff] [review]
patch

The folding for 32-bit could be done by promoting to int64_t rather than double -- but I doubt it matters on any hardware we're likely to encounter. (Might even be slower on x86-32 systems?)

The folding for LIR_addq and LIR_subq is suboptimal, since a double will hold only 53 bits worth of integer, so we could miss folding opportunities for very large values. Probably obscure enough to leave as-is, but we could preflight to get the whole range; to crib the approach from SafeInt, we would have something like



    const int64_t MIN_INT64 = int64_t(0x8000000000000000LL);
    const int64_t MAX_INT64 = int64_t(0x7FFFFFFFFFFFFFFFLL);

    ...
    
    case LIR_addq:
        if ((c1 ^ c2) < 0) 
        {
            // Overflow only possible if both values are positive, or both negative
            if (c2 < 0) 
            {
                // Both negative
                if (c1 < MIN_INT64 - c2)
                    break;
            } 
            else 
            {
                // Both positive
                if (MAX_INT64 - c1 < c2)
                    break;
            }
        }
        return insImmQ(c1 + c2);

    case LIR_subq:
        if ((c1 ^ c2) < 0) 
        {
            // Overflow only possible if both values are positive, or both negative
            if (c2 < 0) 
            {
                // Both negative
                if (c1 < MIN_INT64 + c2)
                    break;
            } 
            else 
            {
                // Both positive
                if (c1 > MAX_INT64 + c2)
                    break;
            }
        }
        return insImmQ(c1 - c2);

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch, v2

Good catch on the 53-bit limit.  64-bit add/sub will almost certainly never overflow in practice because they're only used for pointers -- where we're likely to be adding/subbing a pointer and a small constant -- but we wouldn't want to miss cases where the pointer is near the end of the address space.

I reworked your code.  For subq overflow can only happen if one operand is positive and one negative.  And I replaced the xor tests with simpler comparisons.  And I added some comments that help make it clear that the reasoning is valid.

Comment 5

[Steven Johnson]

Comment on attachment 457134 [details] [diff] [review]
patch, v2

Sweet -- yeah, my code was untested (obviously, since the subq case was wrong); yours looks more plausible.

Comment 6

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/94aa179ec87c

Comment 7

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/411debd27bf1

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/411debd27bf1

Comment 9

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/bdf1256966a5