Bug 194250

Created attachment 361134 [details] Patch

Comment on attachment 361134 [details] Patch Can you add B3 tests for each of these?

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?

(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).

Created attachment 361215 [details] Patch

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.

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

(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.

(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..

Created attachment 361341 [details] Patch

Comment on attachment 361341 [details] Patch Nice. r=me

Comment on attachment 361341 [details] Patch Clearing flags on attachment: 361341 Committed r241126: <https://trac.webkit.org/changeset/241126>

All reviewed patches have been landed. Closing bug.

<rdar://problem/47888576>