Summary: | B3MoveConstants should filter directly on Values, and only create ValueKeys when useful | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robin Morisset <rmorisset> | ||||||||
Component: | JavaScriptCore | Assignee: | Robin Morisset <rmorisset> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Robin Morisset
2021-05-29 11:48:27 PDT
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]. |