Bug 151073

Summary: [B3] Add more tests for Check and fix bugs this found
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch
none
the patch saam: review+

Description Filip Pizlo 2015-11-09 17:53:59 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2015-11-09 17:57:46 PST
Created attachment 265131 [details]
the patch

It's cool to see the MegaCombo test in action:

[pizlo@shakezilla OpenSource] JSC_dumpDisassembly=true JSC_dumpGraphAtEachPhase=true JSC_validateGraphAtEachPhase=true DYLD_FRAMEWORK_PATH=WebKitBuild/Debug/ WebKitBuild/Debug/testb3 CheckMegaCombo
testCheckMegaCombo()...
B3 after initial, before reduceStrength:
BB#0: ; frequency = nan
    Int64 @0 = ArgumentReg(%rdi)
    Int64 @1 = ArgumentReg(%rsi)
    Int32 @2 = Trunc(@1)
    Int64 @3 = ZExt32(@2)
    Int32 @4 = Const32(1)
    Int64 @5 = Shl(@3, @4)
    Int64 @6 = Add(@0, @5)
    Int32 @7 = Load8S(@6, ControlDependent|Reads:Top)
    Int32 @8 = Const32(42)
    Int32 @9 = LessThan(@7, @8)
    Void @10 = Check(@9:Any, generator = 0x10a3eb010, clobbered = [], usedRegisters = [], ExitsSideways)
    Int32 @11 = Const32(0)
    Void @12 = Return(@11, Terminal)
B3 after reduceStrength, before moveConstants:
BB#0: ; frequency = nan
    Int64 @0 = ArgumentReg(%rdi)
    Int64 @1 = ArgumentReg(%rsi)
    Int32 @2 = Trunc(@1)
    Int64 @3 = ZExt32(@2)
    Int32 @4 = Const32(1)
    Int64 @5 = Shl(@3, @4)
    Int64 @6 = Add(@0, @5)
    Int32 @7 = Load8S(@6, ControlDependent|Reads:Top)
    Int32 @8 = Const32(42)
    Int32 @9 = LessThan(@7, @8)
    Void @10 = Check(@9:Any, generator = 0x10a3eb010, clobbered = [], usedRegisters = [], ExitsSideways)
    Int32 @11 = Const32(0)
    Void @12 = Return(@11, Terminal)
B3 after moveConstants, before lowerToAir:
BB#0: ; frequency = nan
    Int64 @0 = ArgumentReg(%rdi)
    Int64 @1 = ArgumentReg(%rsi)
    Int32 @2 = Trunc(@1)
    Int64 @3 = ZExt32(@2)
    Int32 @4 = Const32(1)
    Int64 @5 = Shl(@3, @4)
    Int64 @6 = Add(@0, @5)
    Int32 @7 = Load8S(@6, ControlDependent|Reads:Top)
    Int32 @8 = Const32(42)
    Int32 @9 = LessThan(@7, @8)
    Void @10 = Check(@9:Any, generator = 0x10a3eb010, clobbered = [], usedRegisters = [], ExitsSideways)
    Int32 @11 = Const32(0)
    Void @12 = Return(@11, Terminal)
Air after initial, before eliminateDeadCode:
BB#0: ; frequency = nan
    Move %rsi, %tmp7, @1
    Move %rdi, %tmp1, @0
    Move32 %tmp7, %tmp2, @3
    Move $1, %tmp6, @4
    Move %tmp2, %tmp5, @5
    Lshift64 $1, %tmp5, @5
    Move %tmp1, %tmp4, @6
    Add64 %tmp5, %tmp4, @6
    Move $42, %tmp3, @8
    Patch &Branch8(3), LessThan, (%tmp1,%tmp2,2), $42, @10
    Move $0, %tmp0, @11
    Move $0, %rax, @12
    Ret @12
Specials:
    &Branch8(3): B3::CheckValue lowered to Branch8 with 3 args.
Air after eliminateDeadCode, before spillEverything:
BB#0: ; frequency = nan
    Move %rsi, %tmp7, @1
    Move %rdi, %tmp1, @0
    Move32 %tmp7, %tmp2, @3
    Patch &Branch8(3), LessThan, (%tmp1,%tmp2,2), $42, @10
    Move $0, %rax, @12
    Ret @12
Specials:
    &Branch8(3): B3::CheckValue lowered to Branch8 with 3 args.
Air after spillEverything, before handleCalleeSaves:
BB#0: ; frequency = nan
    Move %rsi, (stack0), @1
    Move %rdi, (stack6), @0
    Move32 (stack0), %rax, @3
    Move %rax, (stack5), @3
    Move (stack6), %rcx, @10
    Move (stack5), %rdx, @10
    Patch &Branch8(3), LessThan, (%rcx,%rdx,2), $42, @10
    Move $0, %rax, @12
    Ret @12
Stack slots:
    stack0: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack1: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack2: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack3: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack4: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack5: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack6: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack7: byteSize = 8, offsetFromFP = 0, kind = Anonymous
Specials:
    &Branch8(3): B3::CheckValue lowered to Branch8 with 3 args.
Air after handleCalleeSaves, before allocateStack:
BB#0: ; frequency = nan
    Move %rsi, (stack0), @1
    Move %rdi, (stack6), @0
    Move32 (stack0), %rax, @3
    Move %rax, (stack5), @3
    Move (stack6), %rcx, @10
    Move (stack5), %rdx, @10
    Patch &Branch8(3), LessThan, (%rcx,%rdx,2), $42, @10
    Move $0, %rax, @12
    Ret @12
Stack slots:
    stack0: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack1: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack2: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack3: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack4: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack5: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack6: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack7: byteSize = 8, offsetFromFP = 0, kind = Anonymous
Specials:
    &Branch8(3): B3::CheckValue lowered to Branch8 with 3 args.
Air after allocateStack, before simplifyCFG:
BB#0: ; frequency = nan
    Move %rsi, -8(%rbp), @1
    Move %rdi, -16(%rbp), @0
    Move32 -8(%rbp), %rax, @3
    Move %rax, -8(%rbp), @3
    Move -16(%rbp), %rcx, @10
    Move -8(%rbp), %rdx, @10
    Patch &Branch8(3), LessThan, (%rcx,%rdx,2), $42, @10
    Move $0, %rax, @12
    Ret @12
Stack slots:
    stack0: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack1: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack2: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack3: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack4: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack5: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack6: byteSize = 8, offsetFromFP = -16, kind = Anonymous
    stack7: byteSize = 8, offsetFromFP = -8, kind = Anonymous
Specials:
    &Branch8(3): B3::CheckValue lowered to Branch8 with 3 args.
Frame size: 16
Air after simplifyCFG, before reportUsedRegisters:
BB#0: ; frequency = nan
    Move %rsi, -8(%rbp), @1
    Move %rdi, -16(%rbp), @0
    Move32 -8(%rbp), %rax, @3
    Move %rax, -8(%rbp), @3
    Move -16(%rbp), %rcx, @10
    Move -8(%rbp), %rdx, @10
    Patch &Branch8(3), LessThan, (%rcx,%rdx,2), $42, @10
    Move $0, %rax, @12
    Ret @12
Stack slots:
    stack0: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack1: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack2: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack3: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack4: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack5: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack6: byteSize = 8, offsetFromFP = -16, kind = Anonymous
    stack7: byteSize = 8, offsetFromFP = -8, kind = Anonymous
Specials:
    &Branch8(3): B3::CheckValue lowered to Branch8 with 3 args.
Frame size: 16
Air after reportUsedRegisters, before generation:
BB#0: ; frequency = nan
    Move %rsi, -8(%rbp), @1
    Move %rdi, -16(%rbp), @0
    Move32 -8(%rbp), %rax, @3
    Move %rax, -8(%rbp), @3
    Move -16(%rbp), %rcx, @10
    Move -8(%rbp), %rdx, @10
    Patch &Branch8(3), LessThan, (%rcx,%rdx,2), $42, @10
    Move $0, %rax, @12
    Ret @12
Stack slots:
    stack0: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack1: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack2: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack3: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack4: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack5: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack6: byteSize = 8, offsetFromFP = -16, kind = Anonymous
    stack7: byteSize = 8, offsetFromFP = -8, kind = Anonymous
Specials:
    &Branch8(3): B3::CheckValue lowered to Branch8 with 3 args.
Frame size: 16
Generated JIT code for B3::Compilation:
    Code at [0x431355e01000, 0x431355e01040):
      0x431355e01000: push %rbp
      0x431355e01001: mov %rsp, %rbp
      0x431355e01004: add $0xfffffffffffffff0, %rsp
      0x431355e01008: mov %rsi, -0x8(%rbp)
      0x431355e0100c: mov %rdi, -0x10(%rbp)
      0x431355e01010: mov -0x8(%rbp), %eax
      0x431355e01013: mov %rax, -0x8(%rbp)
      0x431355e01017: mov -0x10(%rbp), %rcx
      0x431355e0101b: mov -0x8(%rbp), %rdx
      0x431355e0101f: cmp $0x2a, (%rcx,%rdx,2)
      0x431355e01023: jl 0x431355e01035
      0x431355e01029: mov $0x0, %rax
      0x431355e01030: mov %rbp, %rsp
      0x431355e01033: pop %rbp
      0x431355e01034: ret 
      0x431355e01035: mov $0x2a, %eax
      0x431355e0103a: mov %rbp, %rsp
      0x431355e0103d: pop %rbp
      0x431355e0103e: ret 
testCheckMegaCombo(): OK!
[pizlo@shakezilla OpenSource]
Comment 2 WebKit Commit Bot 2015-11-09 18:00:21 PST
Attachment 265131 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:2417:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2418:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2420:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2433:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2433:  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/testb3.cpp:2434:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2434:  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/testb3.cpp:2435:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2436:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2437:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2464:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2465:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2467:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2482:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2482:  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/testb3.cpp:2484:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2484:  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/testb3.cpp:2486:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2488:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2490:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 20 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2015-11-10 10:33:34 PST
Created attachment 265197 [details]
the patch
Comment 4 WebKit Commit Bot 2015-11-10 10:36:15 PST
Attachment 265197 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:2417:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2418:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2420:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2433:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2433:  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/testb3.cpp:2434:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2434:  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/testb3.cpp:2435:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2436:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2437:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2464:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2465:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2467:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2482:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2482:  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/testb3.cpp:2484:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2484:  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/testb3.cpp:2486:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2488:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2490:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 20 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Saam Barati 2015-11-10 10:39:48 PST
Comment on attachment 265197 [details]
the patch

r=me
Comment 6 Filip Pizlo 2015-11-10 11:15:03 PST
Landed in http://trac.webkit.org/changeset/192257