1093356 - Refine the range of constants outside the int32 range

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: range-non-int32-constant.patch

While working on bug 1073928, I noticed that the range computed for constants greater than INT32_MAX or less than INT32_MIN was unnecessarily broad. The attached patch fixes it.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: range-ne-zero-not-negative-zero.patch

One more patch; I noticed that beta nodes for comparisons like |x < 0| would generate beta nodes that included negative zero on both sides. Negative zero is not actually equal to zero, so we can exclude it on the false side.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: range-strict-cmp.patch

Oh, one more one more thing. We can support JSOP_STRICTEQ and JSOP_STRICTNE with beta nodes too.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8516797 [details] [diff] [review]
range-strict-cmp.patch

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

::: js/src/jit/RangeAnalysis.cpp
@@ +251,4 @@
>            case JSOP_EQ:
>              comp.setDouble(bound, bound);
>              break;
> +          case JSOP_STRICTNE:

What about an object that overrides its "valueOf" to -0?
if (obj !== 0) {
    assertEq (obj == -0, false); // Will assert. obj equals to -0.
}

Comment 4

[Dan Gohman [:sunfish]]

The range-non-int32-constant.patch patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8c12e04a14

Comment 5

[Dan Gohman [:sunfish]]

Comment on attachment 8516797 [details] [diff] [review]
range-strict-cmp.patch

(In reply to Hannes Verschore [:h4writer] from comment #3)
> What about an object that overrides its "valueOf" to -0?
> if (obj !== 0) {
>     assertEq (obj == -0, false); // Will assert. obj equals to -0.
> }

JSOP_STRICTEQ can call valueOf while JSOP_EQ can't? In that case, I'll just drop this patch.

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Dan Gohman [:sunfish] from comment #5)
> Comment on attachment 8516797 [details] [diff] [review]
> range-strict-cmp.patch
> 
> (In reply to Hannes Verschore [:h4writer] from comment #3)
> > What about an object that overrides its "valueOf" to -0?
> > if (obj !== 0) {
> >     assertEq (obj == -0, false); // Will assert. obj equals to -0.
> > }
> 
> JSOP_STRICTEQ can call valueOf while JSOP_EQ can't? In that case, I'll just
> drop this patch.

The issue is that on STRICT compare "obj !== 0" is always true, since the types don't match. Even if ToNumber(obj) is 0 or -0.

Comment 7

[Dan Gohman [:sunfish]]

(In reply to Hannes Verschore [:h4writer] from comment #6)
> The issue is that on STRICT compare "obj !== 0" is always true, since the
> types don't match. Even if ToNumber(obj) is 0 or -0.

Can we do a strict compare if the types of the operands are not MIRType_Object, or is there something else easy we can do? The patch eliminates a few instructions in stanford-crypto-sha256-iterative, for example, so it'd be neat if there were a way to fix it.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e8c12e04a14

Comment 9

[Dan Gohman [:sunfish]]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Dan Gohman [:sunfish]]

Attachment: range-strict-cmp.patch

Is this what you suggested earlier? It's possible I misunderstood, since I'm unfamiliar with the semantics of strict comparisons.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8519096 [details] [diff] [review]
range-strict-cmp.patch

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

That looks correct. You can even increase that list to:

> Compare_Int32,
> Compare_Int32MaybeCoerceBoth,
> Compare_Int32MaybeCoerceLHS,
> Compare_Int32MaybeCoerceRHS,
> 
> Compare_Double,
> 
> Compare_DoubleMaybeCoerceLHS,
> Compare_DoubleMaybeCoerceRHS,
> 
> Compare_Float

Please add this test in a functions to MCompare ;) or a static function.

Also don't forget to add testcases that test this. E.g. for placeses where this would be incorrect.

Comment 13

[Dan Gohman [:sunfish]]

Attachment: range-strict-cmp.patch

Cool. I've updated the patch accordingly.

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 8521470 [details] [diff] [review]
range-strict-cmp.patch

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

Awesome, thanks!

Comment 15

[Dan Gohman [:sunfish]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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