[JSC] Support Doubles with B3's Add
Created attachment 265333 [details] Patch
Comment on attachment 265333 [details] Patch r=me
Comment on attachment 265333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265333&action=review > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:172 > + if (m_value->child(1)->isLikeZero()) { Does this do the right thing for -0?
(In reply to comment #3) > Comment on attachment 265333 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265333&action=review > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:172 > > + if (m_value->child(1)->isLikeZero()) { > > Does this do the right thing for -0? Yep :)
(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 265333 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=265333&action=review > > > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:172 > > > + if (m_value->child(1)->isLikeZero()) { > > > > Does this do the right thing for -0? > > Yep :) Can you add a comment about why? :-) For example, does isLikeZero return true for -0? Even if this was documented on the method, it would help to say this here. If it does work for -0, then it would be worth explaining why this handles both -0 and 0 as the other value. A table or something that explains this would help for example if we wanted to try to expand this optimization with some more sophisticated algebra.
(In reply to comment #5) > Can you add a comment about why? :-) > > For example, does isLikeZero return true for -0? Even if this was > documented on the method, it would help to say this here. If it does work > for -0, then it would be worth explaining why this handles both -0 and 0 as > the other value. A table or something that explains this would help for > example if we wanted to try to expand this optimization with some more > sophisticated algebra. If you prefer something more explicit, what do you think of: bool isZero() bool isZeroOrMinusZero() on Value?
(In reply to comment #6) > (In reply to comment #5) > > Can you add a comment about why? :-) > > > > For example, does isLikeZero return true for -0? Even if this was > > documented on the method, it would help to say this here. If it does work > > for -0, then it would be worth explaining why this handles both -0 and 0 as > > the other value. A table or something that explains this would help for > > example if we wanted to try to expand this optimization with some more > > sophisticated algebra. > > If you prefer something more explicit, what do you think of: > bool isZero() > bool isZeroOrMinusZero() > on Value? I think that isLikeZero() essentially means -0, and I'm fine with the name. I believe that your optimization assumes that the following is true: 0 + 0 = 0 0 + -0 = 0 -0 + 0 = -0 -0 + -0 = -0 But I don't think that this is true. Here's what I found, in both jsc and lldb: 0 + 0 = 0 0 + -0 = 0 -0 + 0 = 0 -0 + -0 = -0 In other words, when child(1) is 0, this operation acts as a -0 clean-up filter, which is not the same as identity. When child(1) is -0 then it appears that this is indeed an identity. So, either your optimization is wrong, or you need some comments to describe why it's right. That comment should start with pointing out that this optimization applies to both 0 and -0, and you should explain why it behaves like identity in both cases.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > Can you add a comment about why? :-) > > > > > > For example, does isLikeZero return true for -0? Even if this was > > > documented on the method, it would help to say this here. If it does work > > > for -0, then it would be worth explaining why this handles both -0 and 0 as > > > the other value. A table or something that explains this would help for > > > example if we wanted to try to expand this optimization with some more > > > sophisticated algebra. > > > > If you prefer something more explicit, what do you think of: > > bool isZero() > > bool isZeroOrMinusZero() > > on Value? > > I think that isLikeZero() essentially means -0, and I'm fine with the name. *essentially means 0 or -0.
Comment on attachment 265333 [details] Patch Yeah, looks like I got that wrong. I just trusted the tests without much thinking.
Created attachment 265345 [details] Patch
Comment on attachment 265345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265345&action=review > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:172 > // Turn this: Add(value, zero) > // Into an Identity. > - if (m_value->child(1)->isInt(0)) { > + if (m_value->child(1)->isInt(0) || m_value->child(1)->isNegativeZero()) { It would be great to say in the comment why this only works for -0, so that somebody doesn't just assume this is a bug.
Committed r192347: <http://trac.webkit.org/changeset/192347>