816787 - IonMonkey: be smarter on when to remove negative zero check on ADD

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Since NegativeZero landed, range analysis and truncate analysis landed. Both can be used to remove even more negative zero checks on Add's:

- if add truncates, we don't care if it can be negative zero
- if range analysis shows an operand is > 0, or both operands are positive, we can't have a negative zero ...

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: patch

- an add/sub that truncates doesn't need to distinguish between 0 and -0
- a mul that that truncates doesn't need to distinguish between 0 and -0
- negative zero check can get removed for more cases during range analysis

Comment 2

[Hannes Verschore [:h4writer]]

This removes the last negative zero check on am3 in v8-crypto

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Patch

There was still a small fault in my patch, regarding "updateForReplacement". This is now solved.

Comment 4

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 687399 [details] [diff] [review]
Patch

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

::: js/src/ion/MIR.cpp
@@ +591,1 @@
>      // Test if all uses have the same symantic for -0 and 0

Not part of this commit, but it should be "semantics"

@@ +603,3 @@
>              // x + y gives -0, when both x and y are -0
>              // - When other operand can't produce -0 (i.e. all opcodes, except Mul/Div/ToInt32)
>              //   Remove negative zero check on this operand 

Not part of this commit, but trailing whitespace

@@ +636,5 @@
>            }
> +          case MDefinition::Op_Sub:
> +            if (use_def->toSub()->isTruncated())
> +                break;
> +            /* Fall through...  */ 

Trailing whitespace

@@ +668,5 @@
>            case MDefinition::Op_BitAnd:
>            case MDefinition::Op_BitOr:
>            case MDefinition::Op_BitXor:
>            case MDefinition::Op_Abs:
> +          case MDefinition::Op_TruncateToInt32:

Strange we didn't handle this case before...

@@ +845,5 @@
>  MAdd::updateForReplacement(MDefinition *ins_)
>  {
>      JS_ASSERT(ins_->isAdd());
>      MAdd *ins = ins_->toAdd();
> +    implicitTruncate_ = isTruncated() && ins->isTruncated();

Can you make this use setTruncated?  I've found life is more sane when there is only a single place where member variables are assigned.  You may need a new function clearTruncated as well.

@@ +867,5 @@
>  MSub::updateForReplacement(MDefinition *ins_)
>  {
>      JS_ASSERT(ins_->isSub());
>      MSub *ins = ins_->toSub();
> +    implicitTruncate_ = isTruncated() && ins->isTruncated();

Ditto.

@@ +953,5 @@
>  void
>  MBinaryArithInstruction::infer(JSContext *cx, const TypeOracle::BinaryTypes &b)
>  {
>      // Retrieve type information of lhs and rhs
>      // Rhs is defaulted to int32 first, 

Not part of this commit, but trailing whitespace.

::: js/src/ion/RangeAnalysis.cpp
@@ +480,5 @@
> +    EnsureRange(&lhs);
> +    EnsureRange(&rhs);
> +
> +    // Both values are positive
> +    if (lhs->lower_ >= 0 && rhs->lower_ >= 0)

The >= 0 makes me a bit nervous.  I know I fixed a bug that looked mighty like this in the past.  http://hg.mozilla.org/mozilla-central/rev/4eb7625e4426 . If I remember correctly, that bug came up in jit-tests.  Something like (a * b) * c
where the lower is 0, but it is *actually* -0, and -0 * 1 == -0.  Feel free to ignore this one if I'm mis-remembering.

Comment 5

[Hannes Verschore [:h4writer]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #4)
> The >= 0 makes me a bit nervous.  I know I fixed a bug that looked mighty
> like this in the past. 
> http://hg.mozilla.org/mozilla-central/rev/4eb7625e4426 . If I remember
> correctly, that bug came up in jit-tests.  Something like (a * b) * c
> where the lower is 0, but it is *actually* -0, and -0 * 1 == -0.  Feel free
> to ignore this one if I'm mis-remembering.

Yes, the computed range of the Mul will indeed not know the difference between -0 and 0, but here we are checking the range of the operands of the Mul. I think in this case it isn't a problem, because:
- canBeNegativeZero removal is done on int32 mul
- Operands will use ToInt32 (if they aren't integer) and/or will bail to capture "-0".
=> the 0 of an operand can't be "-0" if we specialized to Integer mul.

Comment 6

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92c4255955d2

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/92c4255955d2