897747 - IonMonkey: Incorrect result with MMod range analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

function f(z)
{
  return (((0x80000000 | 0) % (0x80000001 | z)) | 0) % 100000
}
assertEq(f(0), -1);
assertEq(f(0), -1);

With --ion-eager, the second comparison fails: got 67295, expected -1.

Also affects OdinMonkey.

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/aeff293dadd3
user:        Dan Gohman
date:        Wed Jul 17 08:16:05 2013 -0700
summary:     Bug 891739 - IonMonkey: Make or_ and xor_ range analysis even more precise, and avoid calling js_bitscan_clz32 with a zero operand. r=nbp,waldo

Comment 1

[Dan Gohman [:sunfish]]

It appears to be a bug uncovered in MMod::computeRange(). It is called with lhs=[INT_MIN,INT_MIN] and rhs=[INT_MIN,-1] and it computes a range of [0,0].

Comment 2

[Jeff Walden [:Waldo]]

Yeah, that computation doesn't make any sense when the divisor is fully negative.  I think that could use the same treatment or_/xor_ got, to handle in separate cases a fully-negative divisor, a fully-positive divisor, and a divisor that could be negative or positive.  It also doesn't look like that code handles a potentially infinite rhs correctly.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: mmod-range.patch

This patch assumes Math.abs(x % y) == Math.abs(x) % Math.abs(y), which seems correct, but I'd appreciate confirmation :-).

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 781050 [details] [diff] [review]
mmod-range.patch

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

It'd be good to have a test that gets mis-diagnosed by the |x % Infinity| case, in addition to this one.

Comment 5

[Jeff Walden [:Waldo]]

Actually Math.abs(x % y) is Math.abs(x) % y -- the sign of the result is the sign of the dividend.  But it's all the same as far as this patch is concerned.

Comment 6

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d14c20ba495

Comment 7

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/5d14c20ba495