886246 - IonMonkey: Incorrect result with ~~

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

var f = (function() {
  "use asm"
  function f(x)
  {
    x = x|0;
    return (~~((x ? 1.0e60 : 1.0e60) + 1.0))|0;
  }
  return f;
})();
print(f(1));


With    "use asm": 1
Without "use asm": 0

Comment 2

[Jesse Ruderman]

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/4c0d13ce4c4a
user:        Jan de Mooij
date:        Fri Apr 12 14:16:57 2013 +0200
summary:     Bug 859257 - Mark DoublePun values in ToIntWidth as volatile to work around a Clang bug. r=jwalden

Comment 3

[Jeff Walden [:Waldo]]

That regression is somewhat dubious, because ToIntWidth now uses a completely different algorithm.  It may have introduced the issue, but its existence now has nothing to do with that change.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

The test below suggests that it's not an OdinMonkey bug but also an IonMonkey bug.

Looking at the asmjs generated code, the addition with 1 is made after the conversion of 1.0e60 to an integer is carried out. This suggests that range analysis inverts the position of addition and truncation. The issue is that range analysis doesn't take into account the fact that 1.0e60 + 1 == 1.0e60 (due to precision loss).


// don't launch with --ion-eager
var f = (function() {
  function f(x)
  {
    x = x|0;
    return (~~((x ? 1.0e60 : 1.0e60) + 1.0))|0;
  }
  return f;
})();

var r = -1;
for(var i = 0; i < 20000; i++) {
	r = f();
}
assertEq(r, 0);

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: proposed fix

The problem occurred at this point: (x ? 1.e60 : 1.e60). The range for the resulting Phi node is the merge of the two ranges, created by unionWith.

1.e60 gets the upper_infinite_ flag set as it's not in the range of representable integers; in the mean time, the upper_ value is set to JS_VAL_INTMAX.

unionWith was setting the upper_infinite_ and lower_infinite_ flags before the setLower and setUpper calls. setUpper would replace the upper_infinite_ flag by false. This is wrong.

I propose an unionWith rewrite that takes everything into account. We don't need to set the max exponent by hand as all the other called methods do it for us. Meanwhile, it's not the case for decimal apparently, so I let the corresponding statement.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 767341 [details] [diff] [review]
proposed fix

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

::: js/src/ion/RangeAnalysis.cpp
@@ -354,2 @@
>     decimal_ |= other->decimal_;
> -   max_exponent_ = Max(max_exponent_, other->max_exponent_);

We should keep this result and set it back after setUpper / setLower such as we keep max_exponent as an over approximation of the exponent.

Removing this line will reset the exponent to Int32Exponent and will introduce an other bug where we could assume the result fits on 32 bits if truncated, thus cause an eager truncation which might keep the lower bits where they would have been discarded by double computations.

function noGreedyTruncate(arg, x) {
  var a = arg;
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  if (x) { a += a } else { a += a; a += a }
  a = (a + 1) | 0;

  assertEq(a, (Math.pow(arg, (x ? 16 : 32)) + 1) | 0);
}

::: js/src/jit-test/tests/ion/bug886246.js
@@ +1,1 @@
> +// don't launch with --ion-eager

Why this comment?

@@ +1,2 @@
> +// don't launch with --ion-eager
> +var f = (function() {

is this wrapping function necessary for the test case to fail before the patch?

@@ +2,5 @@
> +var f = (function() {
> +  function f(x)
> +  {
> +    x = x|0;
> +    return (~~((x ? 1.0e60 : 1.0e60) + 1.0))|0;

if the problem is on the (?:), do we need ~~ operators, or |0 operator ?

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: proposed fix + simpler test

Thanks for the review! You were right, the test can be simpler, but not simpler than this one. Applying ~ twice will fit the value into a signed 32 bits int with the expected value.

That being said, the test you gave in the bug (after the assertion fix) doesn't fail with and without this patch. For the record, here is the fixed test:

var f = function noGreedyTruncate(arg, x) {
  var a = arg|0;
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  if (x) { a += a ; a += a} else { a += a }
  a = (a + 1) | 0;
  assertEq( a, (arg * Math.pow(2, (x?16:32)) + 1)|0 )
}

var arg = Math.pow(2,31)|0;
var x = 1;
for(var n = 20000; n > 0; --n) {
    f(arg, x);
}

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cce7728d02dc

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cce7728d02dc