Truncated MMul is emitting an overflow check
Categories
(Core :: JavaScript Engine: JIT, defect, P2)
Tracking
()
Tracking  Status  

firefox111    affected 
People
(Reporter: anba, Unassigned)
References
(Blocks 1 open bug)
Details
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.
Updated•2 years ago

Comment 1•1 year ago


Can I take this? As I understand it, we can refine the bounds to exclude 0
from the range of the multiplication because:
 One of the operands satisfies:
!canHaveFractionalPart() && !canBeNegativeZero()
(ie. whole number or maybe+inf
/NaN
?)  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•1 year ago


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 logicalOR 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•1 year ago


(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 multiplicationimul $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 be0
, it would be truncated to0
by the logicalOR 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 ioneager
) and jittests
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?
Description
•