1810419 - Truncated MMul is emitting an overflow check

Status: NEW

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[André Bargull [:anba]]

Test case:

function m(x) {
  return (x * 3)|0;
}

function f() {
  // Don't Ion compile.
  with ({});

  let xs = [1, 2];
  let y = 0;
  for (let i = 0; i < 10000; ++i) {
    y = m(xs[i & 1]);
  }
  return y;
}

for (let i = 0; i < 10; ++i) {
  print(f());
}

print(disnative(m));

The generated assembly contains an overflow check, even though the result is truncated.

0000001F  6b c0 03                              imul $0x03, %eax, %eax
00000022  0f 80 32 00 00 00                     jo 0x000000000000005A

Range analysis handles negative zero in three places:

• Range::negativeZeroMul.
• The exact same code is duplicated in Range::mul.
• And in MMul::collectRangeInfoPreTrunc.

The negative zero check in Range::mul determines that negative zero is possible, because for lhs = x [int32] and rhs = 3 [int32], the condition lhs->canHaveSignBitSet() && rhs->canBeFiniteNonNegative() is true. That leads to setting newMayIncludeNegativeZero to NegativeZeroFlag::IncludesNegativeZero in the newly computed range. Later on ComputeTruncateKind checks for possible rounding errors (which includes checking Range::canBeNegativeZero()) and then rejects truncation, because negative zero is included in the MMul range.

Comment 1

[Krishna Ravishankar]

Can I take this? As I understand it, we can refine the bounds to exclude -0 from the range of the multiplication because:

1. One of the operands satisfies: !canHaveFractionalPart() && !canBeNegativeZero() (ie. whole number or maybe +-inf/NaN?)
2. While the other satisfies: isFiniteNonNegative() && !canBeZero() (ie. positive?)

Meaning neither operand has -0, there is no underflow and no negative * 0 (or vice versa), so -0 can't be there.

Comment 2

[Nicolas B. Pierron [:nbp]]

Yes, you can take this issue.

To give a bit more context, the problem is the jo instruction which is checking for overflow after the int32 multiplication imul $0x03, eax, eax

The problem is that the | operator which is written in JavaScript is supposed to coerce the number to an integer (truncated) unconditionally. Thus, even if the multiplication results were to be -0, it would be truncated to 0 by the logical-OR operator.

The generation of the overflow check is conditioned by MMul::canOverflow, while we are supposed to know that the result is unconditionally truncated.

The truncated flag is supposed to be set by RangeAnalysis::truncate(), by calling MMul::truncate with TruncateKind::Truncate.

The truncate kind, is computed by ComputeTruncateKind, and the issue reported in comment 0 implies that ComputeRequestedTruncateKind which is supposed to return TruncateKind::Truncate. Thus avoiding the overflow check.

Comment 3

[Krishna Ravishankar]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)

To give a bit more context, the problem is the jo instruction which is checking for overflow after the int32 multiplication imul $0x03, eax, eax

The problem is that the | operator which is written in JavaScript is supposed to coerce the number to an integer (truncated) unconditionally. Thus, even if the multiplication results were to be -0, it would be truncated to 0 by the logical-OR operator.

Thanks for the context! Since it should be truncating regardless of whether -0 is present, I am considering removing the negative zero check from Range::canHaveRoundingErrors to allow ComputeTruncateKind to go on and examine all observable uses for truncation. If I understand correctly, it's not that -0 itself is problematic for truncation, but rather that -0 could be an indication of underflow, and we shouldn't truncate in that case as the inputs/operands could be erroneously truncated? If so, I think removing the negative zero check should be fine, since the fractional part check should still be sufficient to prevent these erroneous truncations.

I saw that the negative zero check was introduced in c2cb21f38ddd2da2760cee8f18316ba5e832d0a8 along with a test. The test, and more generally, jstests (even with --ion-eager) and jit-tests do not have any regressions. Since you were a reviewer then, do you happen to know if I am missing something or if there are additional test suites that I should be running before posting a patch?