Bug 226420 – B3MoveConstants should filter directly on Values, and only create ValueKeys when useful

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

[Robin Morisset]

This is a trivial change: there is a lot of code in that pass that does:
```
ValueKey key(value);
if (!filter(key))
    continue;
```
Replacing it by
```
if (!filter(value))
    continue;
ValueKey key(value)
```
avoids running the constructor of ValueKey in the common case.

This reduces the time spent in B3MoveConstants over a run of JetStream2 from about 160ms to about 105ms, so a slightly less than 1% reduction in the total time spent in B3+Air (about 6s).

[Robin Morisset]

Created attachment 430099 [details]
Patch

[Robin Morisset]

Created attachment 430101 [details]
Patch

fix style nit

[Mark Lam]

Comment on attachment 430101 [details]
Patch

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

> Source/JavaScriptCore/b3/B3MoveConstants.cpp:162
> +                    ValueKey key = child->key();
> +                    ASSERT_UNUSED(key, valueForConstant.get(key) == child);

If key is not used other than just for this assert, can you just do this instead?
    ASSERT(valueForConstant.get(child->key()) == child);

> Source/JavaScriptCore/b3/B3MoveConstants.cpp:329
> +            const auto* constValue = value->as<ConstDoubleValue>();

Why grab the ConstDoubleValue?  Why not just use asDouble() and go straight to the constValue->value()?

> Source/JavaScriptCore/b3/B3MoveConstants.cpp:331
> +            return bitwise_cast<uint64_t>(constValue->value()) != bitwise_cast<uint64_t>(doubleZero);

Why the comparison of the bits?  Why not just do the double compare against 0.0?

> Source/JavaScriptCore/b3/B3MoveConstants.cpp:336
> +            const auto* constValue = value->as<ConstFloatValue>();
> +            float floatZero = 0.0;
> +            return bitwise_cast<uint32_t>(constValue->value()) != bitwise_cast<uint32_t>(floatZero);

Ditto.

[Filip Pizlo]

Comment on attachment 430101 [details]
Patch

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

r=me, though I recommend at least answering Mark's questions before landing.

>> Source/JavaScriptCore/b3/B3MoveConstants.cpp:331
>> +            return bitwise_cast<uint64_t>(constValue->value()) != bitwise_cast<uint64_t>(doubleZero);
> 
> Why the comparison of the bits?  Why not just do the double compare against 0.0?

Probably because -0 and +0 are equal?

[Robin Morisset]

(In reply to Mark Lam from comment #3)
> Comment on attachment 430101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430101&action=review
> 
> > Source/JavaScriptCore/b3/B3MoveConstants.cpp:162
> > +                    ValueKey key = child->key();
> > +                    ASSERT_UNUSED(key, valueForConstant.get(key) == child);
> 
> If key is not used other than just for this assert, can you just do this
> instead?
>     ASSERT(valueForConstant.get(child->key()) == child);

good idea, will do.

> 
> > Source/JavaScriptCore/b3/B3MoveConstants.cpp:329
> > +            const auto* constValue = value->as<ConstDoubleValue>();
> 
> Why grab the ConstDoubleValue?  Why not just use asDouble() and go straight
> to the constValue->value()?

I did not remember about asDouble() existing. Will do.

> 
> > Source/JavaScriptCore/b3/B3MoveConstants.cpp:331
> > +            return bitwise_cast<uint64_t>(constValue->value()) != bitwise_cast<uint64_t>(doubleZero);
> 
> Why the comparison of the bits?  Why not just do the double compare against
> 0.0?

Previously, this code was doing a comparison of the bits, through ValueKey.
I decided not to mess with the semantics of it, especially since doubles have kinda weird rules for equality (-0.0 == +0.0 if I remember correctly, but there might be other madness related to NaN as well)

[Mark Lam]

Comment on attachment 430101 [details]
Patch

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

>>>> Source/JavaScriptCore/b3/B3MoveConstants.cpp:331
>>>> +            return bitwise_cast<uint64_t>(constValue->value()) != bitwise_cast<uint64_t>(doubleZero);
>>> 
>>> Why the comparison of the bits?  Why not just do the double compare against 0.0?
>> 
>> Probably because -0 and +0 are equal?
> 
> Previously, this code was doing a comparison of the bits, through ValueKey.
> I decided not to mess with the semantics of it, especially since doubles have kinda weird rules for equality (-0.0 == +0.0 if I remember correctly, but there might be other madness related to NaN as well)

Ah yes, I missed the part about it comparing ValueKeys.

[Robin Morisset]

Created attachment 430419 [details]
Patch for landing

Applied Mark's suggestions

[EWS]

Committed r278390 (238415@main): <https://commits.webkit.org/238415@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430419 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78791845>