Refine the range of constants outside the int32 range

RESOLVED FIXED in mozilla36

Status

()

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

Trunk
mozilla36
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Attachment #8516315 - Flags: review?(nicolas.b.pierron)
Attachment #8516315 - Flags: review?(hv1989)
Attachment #8516315 - Flags: review?(hv1989) → review+
Attachment #8516315 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8516793 - Flags: review?(nicolas.b.pierron)
Attachment #8516793 - Flags: review?(hv1989)
Posted patch range-strict-cmp.patch (obsolete) — Splinter Review
Oh, one more one more thing. We can support JSOP_STRICTEQ and JSOP_STRICTNE with beta nodes too.
Attachment #8516797 - Flags: review?(nicolas.b.pierron)
Attachment #8516797 - Flags: review?(hv1989)
Attachment #8516793 - Flags: review?(hv1989) → review+
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.
}
Attachment #8516797 - Flags: review?(hv1989)
The range-non-int32-constant.patch patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8c12e04a14
Keywords: leave-open
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.
Attachment #8516797 - Attachment is obsolete: true
Attachment #8516797 - Flags: review?(nicolas.b.pierron)
(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.
(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.
Attachment #8516793 - Flags: review?(nicolas.b.pierron) → review+
Posted patch range-strict-cmp.patch (obsolete) — Splinter Review
Is this what you suggested earlier? It's possible I misunderstood, since I'm unfamiliar with the semantics of strict comparisons.
Attachment #8519096 - Flags: feedback?(hv1989)
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.
Attachment #8519096 - Flags: feedback?(hv1989) → feedback+
Keywords: leave-open
Cool. I've updated the patch accordingly.
Attachment #8519096 - Attachment is obsolete: true
Attachment #8521470 - Flags: review?(nicolas.b.pierron)
Attachment #8521470 - Flags: review?(hv1989)
Comment on attachment 8521470 [details] [diff] [review]
range-strict-cmp.patch

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

Awesome, thanks!
Attachment #8521470 - Flags: review?(hv1989) → review+
Attachment #8521470 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/a45cf456823d
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.