Bug 183543

Created attachment 335516 [details] Patch

Attachment 335516 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3Const32Value.cpp:251: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/b3/B3Const32Value.cpp:274: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/b3/B3Const64Value.cpp:251: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/b3/B3Const64Value.cpp:274: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 335516 [details] Patch I would like to sort operands too.

Comment on attachment 335516 [details] Patch operand sorting is done in bug 150948. So set r?.

It is just considering unsigned values, right? If yes, LGTM. It should be good have some benchmark report.

I'll a bit extend this patch to support arbitrary comparisons.

Created attachment 335535 [details] Patch

(In reply to Caio Lima from comment #5) > It is just considering unsigned values, right? If yes, LGTM. It should be > good have some benchmark report. I don't think it directly improves performance so much. Rather it reduces code size for ArraySlice since some of B3 Values & branches are removed.

Before Int32 @65 = Load(@62, offset = -8, ControlDependent|Reads:4, DFG:@65) Move32 -8(%r9), %rax, @65 0x7fb645720ca5: mov -0x8(%r9), %eax Int32 @0 = Const32(0) Move $0, %rsi, $0(@0) 0x7fb645720ca9: xor %rsi, %rsi Int32 @71 = Above($0(@0), @65, DFG:@65) Int32 @72 = Select(@71, @65, $0(@0), DFG:@65) MoveConditionally32 Below, %rax, $0, %rax, %rsi, %rdi, @72 0x7fb645720cac: cmp $0x0, %eax 0x7fb645720caf: mov %rsi, %rdi 0x7fb645720cb2: cmovb %rax, %rdi Int32 @74 = Sub(@65, @72, DFG:@65) Move32 %rax, %rcx, @74 0x7fb645720cb6: mov %eax, %ecx Sub32 %rdi, %rcx, @74 0x7fb645720cb8: sub %edi, %ecx Int32 @75 = Below(@72, @65, DFG:@65) Int32 @76 = Select(@75, @74, $0(@0), DFG:@65) MoveConditionally32 Below, %rdi, %rax, %rcx, %rsi, %rsi, @76 0x7fb645720cba: cmp %eax, %edi 0x7fb645720cbc: cmovb %rcx, %rsi Int32 @79 = Load8Z(@51, offset = 4, ControlDependent|Reads:19, DFG:@65) Load8 4(%rdx), %rcx, @79 0x7fb645720cc0: movzx 0x4(%rdx), %ecx Int32 @80 = Const32(31, DFG:@65) Int32 @81 = BitAnd(@79, $31(@80), DFG:@65) And32 $31, %rcx, @81 0x7fb645720cc4: and $0x1f, %ecx Int64 @83 = Const64(140420816250832, DFG:@65) Move $0x7fb644df27d0, %rdx, $140420816250832(@83) 0x7fb645720cc7: mov $0x7fb644df27d0, %rdx Int64 @82 = Const64(140420816250720, DFG:@65) Add64 $-112, %rdx, %r10, $140420816250720(@82) 0x7fb645720cd1: lea -0x70(%rdx), %r10 Int32 @84 = Const32(9, DFG:@65) Int32 @85 = Equal(@81, $9(@84), DFG:@65) Int64 @86 = Select(@85, $140420816250832(@83), $140420816250720(@82), DFG:@65) MoveConditionally32 Equal, %rcx, $9, %rdx, %r10, %r10, @86 0x7fb645720cd5: cmp $0x9, %ecx 0x7fb645720cd8: cmovz %rdx, %r10 Int64 @87 = Const64(140420816250608, DFG:@65) Add64 $-224, %rdx, %rax, $140420816250608(@87) 0x7fb645720cdc: lea -0xe0(%rdx), %rax Int32 @88 = Const32(5, DFG:@65) Int32 @89 = Equal(@81, $5(@88), DFG:@65) Int64 @90 = Select(@89, $140420816250608(@87), @86, DFG:@65) MoveConditionally32 Equal, %rcx, $5, %rax, %r10, %r10, @90 0x7fb645720ce3: cmp $0x5, %ecx 0x7fb645720ce6: cmovz %rax, %r10 Int64 @2 = Const64(0) After: Int32 @65 = Load(@62, offset = -8, ControlDependent|Reads:4, DFG:@65) Move32 -8(%r8), %rcx, @65 0x7f0fc0020c84: mov -0x8(%r8), %ecx Int32 @0 = Const32(0) Move $0, %rsi, $0(@0) 0x7f0fc0020c88: xor %rsi, %rsi Int32 @637 = Above(@65, $0(@0), DFG:@65) Int32 @74 = Select(@637, @65, $0(@0), DFG:@65) MoveConditionally32 Above, %rcx, $0, %rcx, %rsi, %rsi, @74 0x7f0fc0020c8b: cmp $0x0, %ecx 0x7f0fc0020c8e: cmova %rcx, %rsi Int32 @77 = Load8Z(@51, offset = 4, ControlDependent|Reads:19, DFG:@65) Load8 4(%rax), %rcx, @77 0x7f0fc0020c92: movzx 0x4(%rax), %ecx Int32 @78 = Const32(31, DFG:@65) Int32 @79 = BitAnd(@77, $31(@78), DFG:@65) And32 $31, %rcx, @79 0x7f0fc0020c96: and $0x1f, %ecx Int64 @81 = Const64(139827205842896, DFG:@65) Add64 $785968, %rdx, %rax, $139827205842896(@81) 0x7f0fc0020c99: lea 0xbfe30(%rdx), %rax Int64 @80 = Const64(139827205842784, DFG:@65) Add64 $785856, %rdx, %r9, $139827205842784(@80) 0x7f0fc0020ca0: lea 0xbfdc0(%rdx), %r9 Int32 @82 = Const32(9, DFG:@65) Int32 @83 = Equal(@79, $9(@82), DFG:@65) Int64 @84 = Select(@83, $139827205842896(@81), $139827205842784(@80), DFG:@65) MoveConditionally32 Equal, %rcx, $9, %rax, %r9, %r9, @84 0x7f0fc0020ca7: cmp $0x9, %ecx 0x7f0fc0020caa: cmovz %rax, %r9 Int64 @85 = Const64(139827205842672, DFG:@65) Add64 $785744, %rdx, %rax, $139827205842672(@85) 0x7f0fc0020cae: lea 0xbfd50(%rdx), %rax Int32 @86 = Const32(5, DFG:@65) Int32 @87 = Equal(@79, $5(@86), DFG:@65) Int64 @88 = Select(@87, $139827205842672(@85), @84, DFG:@65) MoveConditionally32 Equal, %rcx, $5, %rax, %r9, %r9, @88 0x7f0fc0020cb5: cmp $0x5, %ecx 0x7f0fc0020cb8: cmovz %rax, %r9 Int64 @2 = Const64(0)

Created attachment 335536 [details] Patch

Created attachment 335537 [details] Patch

Created attachment 335539 [details] Patch

Comment on attachment 335539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335539&action=review Nice! > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1676 > - auto* value = newComparisonVaueIfNecessary(); > + CanonicalizedComparison comparison = canonicalizeComparison(m_value); I know the auto* is in the red, but I sure hope we don't have more cases of auto being used as an alias for Value. It's important for the compiler to be self-documenting, and that means having types explicitly.

Comment on attachment 335539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335539&action=review Thanks >> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1676 >> + CanonicalizedComparison comparison = canonicalizeComparison(m_value); > > I know the auto* is in the red, but I sure hope we don't have more cases of auto being used as an alias for Value. It's important for the compiler to be self-documenting, and that means having types explicitly. Yeah, it sounds reasonable to me.

Comment on attachment 335539 [details] Patch Clearing flags on attachment: 335539 Committed r229517: <https://trac.webkit.org/changeset/229517>

All reviewed patches have been landed. Closing bug.

<rdar://problem/38397886>