...
Created attachment 431934 [details] Patch
Comment on attachment 431934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431934&action=review Nice. r=me > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2761 > + uint64_t width = static_cast<uint64_t>(WTF::bitCount(mask)); nit: this static cast isn't needed > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2795 > + uint64_t width = static_cast<uint64_t>(WTF::bitCount(mask)); nit: this static_cast isn't needed
Comment on attachment 431934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431934&action=review > Source/WTF/wtf/MathExtras.h:741 > +inline size_t countTrailingZeros(uint32_t v) > +{ > + static const unsigned Mod37BitPosition[] = { > + 32, 0, 1, 26, 2, 23, 27, 0, 3, 16, 24, 30, 28, 11, 0, 13, > + 4, 7, 17, 0, 25, 22, 31, 15, 29, 10, 12, 6, 0, 21, 14, 9, > + 5, 20, 8, 19, 18 > + }; > + return Mod37BitPosition[((1 + ~v) & v) % 37]; > +} > + > +inline size_t countTrailingZeros(uint64_t v) should we use builtin_ctz when compiling with GCC or clang so the proper instruction can be selected?
Created attachment 431939 [details] Patch
Comment on attachment 431939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431939&action=review > Source/JavaScriptCore/ChangeLog:19 > + d = (n << lsb) & mask Why not instead select (n & mask) << lsb? Then, you can canonicalize (n << lsb) & mask to (n & mask) << lsb in reduceStrength. The benefit of (n & mask) << lsb being the canonical form is it's easier to recognize. So, if some other part of the compiler needed to recognize this pattern, they'd only have to recognize the easier one of them.
Created attachment 432014 [details] Patch
Comment on attachment 432014 [details] Patch Let's figure out the crash
Created attachment 432059 [details] Patch
Comment on attachment 432059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432059&action=review This looks good but you're matching rules for BitAnd that won't happen after reduceStrength does handleCommutativity. > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2753 > + if (imm(srcValue) || !imm(lsbValue) || lsbValue->asInt() < 0 || !right->hasInt()) You don't need to exit if srcValue has an imm. If it had an imm, then it wouldn't have had ZShr as its opcode. > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2770 > + if (tryAppendUBFX(left, right) || tryAppendUBFX(right, left)) You can be sure that BitAnd will have its immediate on the right. I believe we have a reduceStrength rule for that: case BitAnd: handleCommutativity(); handleCommutativity() will flip the BitAnd to put the immediate on the right if it wasn't on the right already. > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2829 > + if (imm(nValue) || !maskValue->hasInt() || !imm(right) || right->asInt() < 0) No need to bail out of imm() returns true. If both sides are imm then the whole thing would have been constant folded and you wouldn't have even seen a BitAnd. And if it wasn't constant folded for any reason, then there's no downside to you matching this rule. > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2846 > + return tryAndValueMask(andLeft, andRight) || tryAndValueMask(andRight, andLeft); Ditto - only have to match the case where the immediate is on the right.
Created attachment 432089 [details] Patch
Thanks for the review!
Committed r279193 (239084@main): <https://commits.webkit.org/239084@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432089 [details].