Bug 194250 – B3ReduceStrength: missing peephole optimizations for Neg and Sub

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Morisset]

In particular:
- Sub(x, x) => 0
- Sub(x1, Neg(x2)) => Add (x1, x2)
- Abs(Neg(x)) => Abs(x)
- Neg(Sub(x1, x2)) => Sub(x2, x1)
- Add(x1, Neg(x2)) => Sub(x1, x2)
- Add(Neg(x1), x2) => Sub(x2, x1)

I don't expect the performance benefit to be significant, but this is very low-hanging fruit.

[Robin Morisset]

Created attachment 361134 [details]
Patch

[Saam Barati]

Comment on attachment 361134 [details]
Patch

Can you add B3 tests for each of these?

[Saam Barati]

Comment on attachment 361134 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361134&action=review

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:584
> +                // Turn this: Integer Add(value, Neg(otherValue))
> +                // Into this: Sub(value, otherValue)

Does this actually need to only be done on Int types?

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:591
> +                // Turn this: Integer Add(Neg(value), otherValue)
> +                // Into this: Sub(otherValue, value)

Ditto

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:642
> +                // Turn this: Sub(value, Neg(otherValue))
> +                // Into this: Add(value, otherValue)

Can this also be done for floating point types?

[Robin Morisset]

(In reply to Saam Barati from comment #3)
> Comment on attachment 361134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361134&action=review
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:584
> > +                // Turn this: Integer Add(value, Neg(otherValue))
> > +                // Into this: Sub(value, otherValue)
> 
> Does this actually need to only be done on Int types?
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:591
> > +                // Turn this: Integer Add(Neg(value), otherValue)
> > +                // Into this: Sub(otherValue, value)
> 
> Ditto
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:642
> > +                // Turn this: Sub(value, Neg(otherValue))
> > +                // Into this: Add(value, otherValue)
> 
> Can this also be done for floating point types?

I am not entirely sure, and I decided to play it safe. The following sentence in particular scares me:
"Because of negative zero (and also when the rounding mode is upward or downward), the expressions −(x − y) and (−x) − (−y), for floating-point variables x and y, cannot be replaced by y − x. However (−0) + x can be replaced by x with rounding to nearest (except when x can be a signaling NaN)." (from https://en.wikipedia.org/wiki/Signed_zero).
I am not familiar enough with the subtleties of floating point arithmetic to try to do anything smart with floats. While checking again, I realized I made the mistake above (i.e. forgot checking the type for Neg(Sub(x1, x2)) => Sub(x2, x1).

I will submit soon a new patch with this bug fixed, and tests (I did not know about testB3.cpp, it should make it easy).

[Robin Morisset]

Created attachment 361215 [details]
Patch

[Robin Morisset]

Note: There are exactly two optimizations affecting floats/doubles in this patch:
- Abs(Neg(x)) => Abs(x)
  This seems right to me, as Abs(x) always remove the sign bit and Neg just flips it, so even things like negative 0 should not be able to cause any trouble.
- Sub(x, x) => +0.0
   Based on wikipedia: "x-x=x+(-x)=+0 (for any finite x, −0 when rounding toward negative)"
    I assumed that we don't mess with the rounding mode, and it passed the tests. But if we ever decide to round towards negative, we will have to change/remove this optimization.

[Saam Barati]

Comment on attachment 361215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361215&action=review

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:617
> +            // Turn this: Sub(value, value)

I don’t think this is correct for NaN, right?
I think NaN - NaN = NaN

[Robin Morisset]

(In reply to Saam Barati from comment #7)
> Comment on attachment 361215 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361215&action=review
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:617
> > +            // Turn this: Sub(value, value)
> 
> I don’t think this is correct for NaN, right?
> I think NaN - NaN = NaN

Good catch, I will remove this case. But I must first understand why it was not caught by the tests.
It should have been found by testSubArgDouble, as it is called on PNaN through 
RUN_UNARY(testSubArgDouble, floatingPointOperands<double>());
as floatingPointOperands<double> includes PNaN.

So I added some instrumentation.. and it appears that testB3 does not ever call B3ReduceStrength :-(. I'm currently looking into it.

[Robin Morisset]

(In reply to Robin Morisset from comment #8)
> (In reply to Saam Barati from comment #7)
> > Comment on attachment 361215 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=361215&action=review
> > 
> > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:617
> > > +            // Turn this: Sub(value, value)
> > 
> > I don’t think this is correct for NaN, right?
> > I think NaN - NaN = NaN
> 
> Good catch, I will remove this case. But I must first understand why it was
> not caught by the tests.
> It should have been found by testSubArgDouble, as it is called on PNaN
> through 
> RUN_UNARY(testSubArgDouble, floatingPointOperands<double>());
> as floatingPointOperands<double> includes PNaN.
> 
> So I added some instrumentation.. and it appears that testB3 does not ever
> call B3ReduceStrength :-(. I'm currently looking into it.

I had simply forgotten to set DYLD_FRAMEWORK_PATH before running testB3, so it was running some system version of jsc, instead of my broken one..

[Robin Morisset]

Created attachment 361341 [details]
Patch

[Saam Barati]

Comment on attachment 361341 [details]
Patch

Nice. r=me

[WebKit Commit Bot]

Comment on attachment 361341 [details]
Patch

Clearing flags on attachment: 361341

Committed r241126: <https://trac.webkit.org/changeset/241126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47888576>