Closed Bug 886246 Opened 9 years ago Closed 9 years ago

IonMonkey: Incorrect result with ~~

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jruderman, Assigned: bbouvier)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file, 1 obsolete file)

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
Duplicate of this bug: 886248
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
Blocks: 859257
Keywords: regression
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.
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);
Summary: OdinMonkey: Incorrect result with ~~ → IonMonkey: Incorrect result with ~~
Attached patch proposed fix (obsolete) — Splinter Review
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.
Assignee: general → bbouvier
Status: NEW → ASSIGNED
Attachment #767341 - Flags: review?(nicolas.b.pierron)
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 ?
Attachment #767341 - Flags: review?(nicolas.b.pierron)
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);
}
Attachment #767341 - Attachment is obsolete: true
Attachment #768613 - Flags: review?(nicolas.b.pierron)
Attachment #768613 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/cce7728d02dc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.