892794 - IonMonkey: missing truncation on subtract from -INT32_MIN

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dan Gohman [:sunfish]]

The following JavaScript:

function foo(v) {
  return (2147483648-Math.max(1.1,-(((2<<(-v|v))-3)|0)))|0;
}
print(foo(1.6));
print(foo(437348122.9));

prints 2147483647 on the second line in --ion-eager mode, and 2147483646 in --no-ion mode.

The outer-most subtract is to evaluate 2147483648 - 1.1 and get 2147483646.9, which should truncate to 2147483646.

Comment 1

[Dan Gohman [:sunfish]]

In the second call to foo, Math.max returns 1.1, which hits an Int32 type barrier before the subtract, which looks fine. That causes the program to launch off into a bailout path through baseline that I haven't traced through yet, but it ends up computing a floating-point subtract with -2147483648.0 as the left operand.

Apparently baseline knows it needs to do a floating-point subtract, but it's still truncating the operands.

Comment 2

[Dan Gohman [:sunfish]]

Ion is speculating that the operands to the subtract will be integer values. Since the subtract's result is truncated, this lets it truncate its operands. The lhs is a constant (2147483648), which it truncates to -2147483648, which is correct as long as the speculation holds. The rhs is a call, which it wraps in an Int32 type barrier to check the speculation. Then, the call actually returns 1.1, which fails the type barrier, triggering a bailout. However, it looks like the bailout's snapshot uses the truncated lhs, which is incorrect once the type barrier fails. It should use the original lhs instead.

With IONFLAGS=bl-bails, the problematic bailout has this:

      pushing 2 expression stack slots
      WRITE_VAL 0x372a5b8/0x7fffeb438560 StackValue      fff8800080000000
      WRITE_VAL 0x372a5b0/0x7fffeb438558 StackValue      3ff199999999999a

fff8800080000000 is -2147483648 (aka INT32_MIN) with an Int32 tag encoding, and it is the wrong value for the lhs of the subtract. The lhs is supposed to be positive 2147483648, which can't be represented as an Int32.

3ff199999999999a is 1.1, which is the correct rhs for the subtract.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #2)
>       pushing 2 expression stack slots
>       WRITE_VAL 0x372a5b8/0x7fffeb438560 StackValue      fff8800080000000
>       WRITE_VAL 0x372a5b0/0x7fffeb438558 StackValue      3ff199999999999a
> 
> fff8800080000000 is -2147483648 (aka INT32_MIN) with an Int32 tag encoding,
> and it is the wrong value for the lhs of the subtract. The lhs is supposed
> to be positive 2147483648, which can't be represented as an Int32.

Hum … So the problem is that we truncate the constant in-place, and the truncate is then captured by the resume point & snapshot and the bailout restore the truncated version of the constant.

Comment 4

[Dan Gohman [:sunfish]]

This is a simpler testcase which shows similar behavior:

function foo(v) {
  return (2147483648+Math.min(v,0))|0;
}
print(foo(2.1))
print(foo(-0.1))

Comment 5

[Nicolas B. Pierron [:nbp]]

If we want to keep any useful use of range analysis across resume points, then we need to capture the result of the operation before it is seen as a resume point.  Strangely, I fail to reproduce it with the bailout function.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> If we want to keep any useful use of range analysis across resume points,
> then we need to capture the result of the operation before it is seen as a
> resume point.  Strangely, I fail to reproduce it with the bailout function.

Math.mion is in fact needed because we monitor the result of the call(*) and determine that Math.min will always return a number which fit in the Int range.  So we add a type barrier to ensure that the result of the call is an int.  When we got -0.1 as input, we fail the type barrier and go into an invalidating bailout.  The bailout then captures the truncated expression, which in this case is the constant which has been truncated in-place.

This prove that, to be safe we should not modify any of the operands of a resume point, if the result is observable.  The result is observable if any assumption that we made during the generation of the MIR can change the truncation decision. In this case, this is the fact that we cross a resume point after assuming that the truncation which flow into the same operation is only an Integer an that we do not have to truncate it.  Be aware that this is not the only case, and that if we remove a branch with UCE.  So identically we should not truncate across block boundaries, which is extremely sad.

(*) Math.min is not inlined, because 2.1 is seen as a double, even if the result is monitored as an int.

Comment 7

[Nicolas B. Pierron [:nbp]]

I failed to reproduce the failure with the test case from comment 4 (ran with --ion-eager --ion-parallel-compile=off)

Still, we could optimize ranges across resume points, by using RInstructions. (see Bug 878503)

Comment 8

[Dan Gohman [:sunfish]]

Attachment: truncate-subtract.patch

The testcases no longer fail for me either. It appears that what happens at the bailout has changed, though I don't know specifically which patch fixed it. Attached is a patch which simply adds these tests to the testsuite to ensure that they remain passing.

Comment 9

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee388876965

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ee388876965