Truncate intish operations if their uses are truncated

NEW
Unassigned

Status

()

Core
JavaScript Engine
5 years ago
4 years ago

People

(Reporter: nmatsakis, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Right now, if you have code like `(a+b)|0`, where `a` and `b` are integers, we will still bailout if the result overflows, although this is not necessary.
(Reporter)

Comment 1

5 years ago
Created attachment 772249 [details] [diff] [review]
truncate intish operations if their uses are truncated
(Reporter)

Updated

5 years ago
Attachment #772249 - Flags: review?(mrosenberg)
(Reporter)

Comment 2

5 years ago
Correction: `(a+b)|0` probably works today, but `(a/b)|0` does not.
(Reporter)

Updated

5 years ago
Attachment #772249 - Flags: review?(mrosenberg) → review?(bhackett1024)
(Reporter)

Comment 3

5 years ago
Related: bug 866670
Comment on attachment 772249 [details] [diff] [review]
truncate intish operations if their uses are truncated

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

::: js/src/ion/RangeAnalysis.cpp
@@ +1756,5 @@
> +            hasNoRoundingErrors = r && !r->hasRoundingErrors();
> +
> +            // To be truncated, must either be intish or have no
> +            // rounding errors.
> +            if (!isIntishOp && !hasNoRoundingErrors)

Considering that this optimization should only help a/b, this is a lot of new code which is mostly redundant with how the range analysis already works.

Ideally, this could be done by adding an MDiv::computeRange able to indicate that for no integers will a/b produce a value with rounding errors, but that doesn't seem possible with the current representation as a/b can be infinity.

Instead, can you just tailor a special case here for integer division, with a comment explaining what's going on in this specific case rather than the obfuscating intish stuff?
Attachment #772249 - Flags: review?(bhackett1024)
Depends on: 891534
(Reporter)

Comment 5

5 years ago
Created attachment 773163 [details] [diff] [review]
truncate div operations if their uses are truncated
Attachment #772249 - Attachment is obsolete: true
Attachment #773163 - Flags: review?(bhackett1024)
Comment on attachment 773163 [details] [diff] [review]
truncate div operations if their uses are truncated

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

::: js/src/ion/RangeAnalysis.cpp
@@ +1726,4 @@
>                default:;
>              }
>  
> +            // Set truncated flag if either (1) range analysis ensure

Maybe fix the existing grammar issue.

@@ +1727,5 @@
>              }
>  
> +            // Set truncated flag if either (1) range analysis ensure
> +            // that it has no rounding errors and no fractional part
> +            // or (2) this is integer division, which has no range.

This comment should describe why the special case is needed: the result of integer division can be infinite, but cannot be a non-infinite double large enough to have rounding errors, and our representation of ranges cannot capture this property.

@@ +1732,4 @@
>              const Range *r = iter->range();
> +            bool hasNoRoundingErrors = r && !r->hasRoundingErrors();
> +            bool isIntegerDiv =
> +                iter->op() == MDefinition::Op_Div &&

iter->isDiv()

@@ +1732,5 @@
>              const Range *r = iter->range();
> +            bool hasNoRoundingErrors = r && !r->hasRoundingErrors();
> +            bool isIntegerDiv =
> +                iter->op() == MDefinition::Op_Div &&
> +                iter->toBinaryArithInstruction()->specialization() == MIRType_Int32;

iter->toDiv()
Attachment #773163 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

4 years ago
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.