Bug 151164 – [JSC] Support Doubles with B3's Add

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[JSC] Support Doubles with B3's Add

[Benjamin Poulain]

Created attachment 265333 [details]
Patch

[Geoffrey Garen]

Comment on attachment 265333 [details]
Patch

r=me

[Filip Pizlo]

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?

[Benjamin Poulain]

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

[Filip Pizlo]

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

[Benjamin Poulain]

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

[Filip Pizlo]

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

[Filip Pizlo]

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

[Benjamin Poulain]

Comment on attachment 265333 [details]
Patch

Yeah, looks like I got that wrong.
I just trusted the tests without much thinking.

[Benjamin Poulain]

Created attachment 265345 [details]
Patch

[Filip Pizlo]

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.

[Benjamin Poulain]

Committed r192347: <http://trac.webkit.org/changeset/192347>