BitAnd is distributive over both BitOr and BitXor. So the following should all work: - Op(BitAnd(x1, x2), BitAnd(x1, x3)) => BitAnd(x1, Op(x2, x3)) - Op(BitAnd(x2, x1), BitAnd(x1, x3)) => BitAnd(x1, Op(x2, x3)) - Op(BitAnd(x1, x2), BitAnd(x3, x1)) => BitAnd(x1, Op(x2, x3)) - Op(BitAnd(x2, x1), BitAnd(x3, x1)) => BitAnd(x1, Op(x2, x3)) for Op in {BitOr, BitXor} Similarily, we can use De Morgan laws to float binary not (represented as BitXor(x, allOnes)) outwards, and eliminate as many as possible: - BitAnd(BitXor(x1, allOnes), BitXor(x2, allOnes)) => BitXor(BitOr(x1, x2), allOnes) - BitOr(BitXor(x1, allOnes), BitXor(x2, allOnes) => BitXor(BitAnd(x1, x2), allOnes) - BitAnd(BitXor(x, allOnes), c) => BitXor(BitOr(x, ~c)) - BitOr(BitXor(x, allOnes), c) => BitXor(BitAnd(x, ~c)) Like all other peephole optimizations, the performance benefit is probably too small to measure, but it is very low-hanging fruit.
Created attachment 361240 [details] WIP_missing_tests
Attachment 361240 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:999: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1002: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1003: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1003: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1005: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1006: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1016: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1019: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1019: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1021: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1022: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1076: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1079: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1080: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1080: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1082: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1083: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1093: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1096: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1096: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1098: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1099: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2256: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 23 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 361312 [details] Patch
Attachment 361312 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1002: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1019: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1079: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1096: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Build Bot from comment #4) > Attachment 361312 [details] did not pass style-queue: > > > ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1002: When wrapping a > line, only indent 4 spaces. [whitespace/indent] [3] > ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1019: When wrapping a > line, only indent 4 spaces. [whitespace/indent] [3] > ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1079: When wrapping a > line, only indent 4 spaces. [whitespace/indent] [3] > ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1096: When wrapping a > line, only indent 4 spaces. [whitespace/indent] [3] > Total errors found: 4 in 3 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. I don't see a way to solve these without making the condition unreadable. Does it bother anybody if I keep it as is?
Comment on attachment 361312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361312&action=review r=me > Source/JavaScriptCore/ChangeLog:47 > + I did not list every single reordering of the arguments as the test part of this patch is already significantly longer than the useful code, and this combination should sample all the relevant paths. That’s not our criteria for adding tests. Why don’t you just make it so your test doesn’t hard code lhs/rhs to generate all combinations? > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1002 > + && m_value->child(0)->child(1)->isInt(0xffffffffffffffff) maybe use numeric_limits here? > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2242 > + // Turn any of these: You should say for op Or/Xor
Thanks for the review. > > Source/JavaScriptCore/ChangeLog:47 > > + I did not list every single reordering of the arguments as the test part of this patch is already significantly longer than the useful code, and this combination should sample all the relevant paths. > > That’s not our criteria for adding tests. Why don’t you just make it so your > test doesn’t hard code lhs/rhs to generate all combinations? Done. > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1002 > > + && m_value->child(0)->child(1)->isInt(0xffffffffffffffff) > > maybe use numeric_limits here? Done. > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2242 > > + // Turn any of these: > > You should say for op Or/Xor Fixed.
Created attachment 362764 [details] Patch
Comment on attachment 362764 [details] Patch Clearing flags on attachment: 362764 Committed r241964: <https://trac.webkit.org/changeset/241964>
All reviewed patches have been landed. Closing bug.
<rdar://problem/48325164>