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).
Created attachment 430099 [details] Patch
Created attachment 430101 [details] Patch fix style nit
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.
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?
(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)
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.
Created attachment 430419 [details] Patch for landing Applied Mark's suggestions
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].
<rdar://problem/78791845>