Bug 162349 - B3 should have comprehensive support for atomic operations
Summary: B3 should have comprehensive support for atomic operations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 162343 168833
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-21 11:12 PDT by Filip Pizlo
Modified: 2017-03-10 09:50 PST (History)
6 users (show)

See Also:


Attachments
it begins (8.55 KB, patch)
2017-02-21 21:39 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (18.07 KB, patch)
2017-02-23 13:56 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (29.89 KB, patch)
2017-02-23 16:01 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (36.71 KB, patch)
2017-02-24 20:53 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
added the instructions to Air (43.95 KB, patch)
2017-02-25 11:28 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
added isel for fences (51.09 KB, patch)
2017-02-25 12:01 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
starting to work on isel for CAS (52.72 KB, patch)
2017-02-25 14:20 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
built some tools I'll need for lowering CAS to LL/SC in isel (77.45 KB, patch)
2017-02-26 09:17 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
wrote some more isel (101.69 KB, patch)
2017-02-26 12:57 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (107.03 KB, patch)
2017-02-26 16:22 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (118.41 KB, patch)
2017-02-27 09:53 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
done with B3->Air isel for all atomics (131.15 KB, patch)
2017-02-27 16:49 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
adding X86 instruction formatting for atomics (133.73 KB, patch)
2017-02-27 20:17 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more x86 atomic instruction formatting (144.50 KB, patch)
2017-02-27 21:46 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
so many x86 instructions (164.91 KB, patch)
2017-02-28 09:35 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
just when I thought I was done with x86 (201.75 KB, patch)
2017-02-28 10:48 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more x86 (212.25 KB, patch)
2017-02-28 11:47 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it's starting to compile (238.44 KB, patch)
2017-02-28 12:08 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more things compile (244.58 KB, patch)
2017-02-28 12:37 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
maybe the x86 part compiles now (250.46 KB, patch)
2017-02-28 13:52 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it just passed some WeakCAS tests! (264.42 KB, patch)
2017-02-28 17:28 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
added tests for strong cas (272.01 KB, patch)
2017-03-01 10:33 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
added acq/rel tests (282.95 KB, patch)
2017-03-01 13:37 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
x86 looks solid (292.66 KB, patch)
2017-03-01 17:40 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
rebased (292.66 KB, patch)
2017-03-01 20:52 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more ARM64 (318.36 KB, patch)
2017-03-02 14:06 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (325.91 KB, patch)
2017-03-02 18:03 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (333.12 KB, patch)
2017-03-03 11:15 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
almost done arm wrestling (340.61 KB, patch)
2017-03-03 17:03 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
passes all tests on arm (344.38 KB, patch)
2017-03-03 18:00 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
voila! (374.29 KB, patch)
2017-03-04 10:13 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (374.46 KB, patch)
2017-03-04 10:20 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
fix some builds (379.52 KB, patch)
2017-03-04 13:29 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more build fixes (379.55 KB, patch)
2017-03-04 19:52 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
try again to fix gtk build (379.54 KB, patch)
2017-03-04 20:28 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (383.65 KB, patch)
2017-03-06 20:11 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (383.62 KB, patch)
2017-03-09 09:42 PST, Filip Pizlo
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-09-21 11:12:01 PDT
On ARM, we can mark loads and stores as having acquire or release semantics.  B3 should know about this.

I believe that this means that a load-acquire will have a write heap range.  This range must be non-empty.  This achieves both goals of acquire: it means that this load itself is not hoistable, and it means that other loads cannot be hoisted above this load, if they overlap with the write range.

I believe that this means that a store-release will have a read heap range.  This range must be non-empty.  The read range prevents other stores from being sunk below this one.  Also, the write range must be expanded to include anything that we don't want this store to be sunk to.

Maybe we can model acq/rel by just giving MemoryValue a second range?  That would be weird but it seems like it would work.  Alternatively, we would have a B3FencedMemoryValue, which includes the second range.  This second range would behave either as the read range or the write range depending on whether this was a load or a store.
Comment 1 JF Bastien 2016-09-21 11:48:33 PDT
If load acquire has a range and the range is small then you can even reduce it to a consume dependency as you talk about in 162350.
Comment 2 Filip Pizlo 2016-10-30 09:43:38 PDT
(In reply to comment #1)
> If load acquire has a range and the range is small then you can even reduce
> it to a consume dependency as you talk about in 162350.

I don't think that the optimizer can do that.  Consider this program, where B3 only sees procedure bar().

foo()
{
    a = load
}

bar()
{
    b = load
    c = load acquire
}

foo()
bar()

In this program, it's not valid to transform bar() into:

bar()
{
    b = load
    c = load depend b
}

because this does not create a dependency to foo()'s a = load.
Comment 3 Filip Pizlo 2016-10-30 10:13:07 PDT
I think it's important to review the optimizations we want to be able to do.

On x86, we want to be able to optimize store-load fences by transforming this:

store
fenceRW
load

Into this:

atomic store
load

We also want to turn Depend into a load-load fence, which reduces to no-op later.

On ARM we want to be able to optimize fences also.  For example, we want to optimize load-load fences into load acquires:

load
fenceW
load

into:

load
load acquire

We want to be able to optimize store-load fences like this:

store
fenceRW
load

into:

store release
load acquire

Conversely, we want to be able to lower acq/rel flags.  I believe that we want both store and load to accept such flags.  That means that if we have a store acquire or load release, we want to be able to insert fences.
Comment 4 Filip Pizlo 2016-10-30 10:26:50 PDT
I wonder if the right way to optimize this is with some kind of flow algorithm, or by just doing an on-demand analysis on a per-fence basis.  I bet that the latter is better.
Comment 5 Filip Pizlo 2017-02-21 21:39:00 PST
Created attachment 302367 [details]
it begins
Comment 6 Filip Pizlo 2017-02-23 12:10:39 PST
Incredibly, we could represent acq/rel using just a phantomRange in MemoryValue.  If it's bottom, then we assume no acq/rel.  If it's not bottom, then we assume:

- acq for loads.
- rel for stores.
- acq/rel for atomics.
Comment 7 JF Bastien 2017-02-23 12:20:13 PST
What would "no acq/rel" mean? seq_cst, or non-atomic?
Comment 8 Filip Pizlo 2017-02-23 12:35:50 PST
(In reply to comment #7)
> What would "no acq/rel" mean? seq_cst, or non-atomic?

Short story: it means that the load/store behaves like on ARM without acq/rel flags.

The longer story involves B3's effects-based memory model, which can be a bit weaker than ARM if you use abstract heaps in certain ways.
Comment 9 JF Bastien 2017-02-23 12:38:37 PST
OK, and then we wouldn't have seq_cst? Fences would serve that purpose instead (where they're either acq, rel, or acq_rel)?

I think this works, but I'd need to give it more thought to be sure. Sounds good for now though.
Comment 10 Filip Pizlo 2017-02-23 12:39:51 PST
(In reply to comment #9)
> OK, and then we wouldn't have seq_cst? Fences would serve that purpose
> instead (where they're either acq, rel, or acq_rel)?
> 
> I think this works, but I'd need to give it more thought to be sure. Sounds
> good for now though.

How would seq_cst differ from acq_rel in this context?
Comment 11 JF Bastien 2017-02-23 13:19:54 PST
(In reply to comment #10)
> (In reply to comment #9)
> > OK, and then we wouldn't have seq_cst? Fences would serve that purpose
> > instead (where they're either acq, rel, or acq_rel)?
> > 
> > I think this works, but I'd need to give it more thought to be sure. Sounds
> > good for now though.
> 
> How would seq_cst differ from acq_rel in this context?

Oh I was thinking about x86 seq_cst stores, but I guess you already have ortop for that? Doesn't seem useful to have seq_cst at all then :)
Comment 12 Filip Pizlo 2017-02-23 13:54:50 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > OK, and then we wouldn't have seq_cst? Fences would serve that purpose
> > > instead (where they're either acq, rel, or acq_rel)?
> > > 
> > > I think this works, but I'd need to give it more thought to be sure. Sounds
> > > good for now though.
> > 
> > How would seq_cst differ from acq_rel in this context?
> 
> Oh I was thinking about x86 seq_cst stores, but I guess you already have
> ortop for that? Doesn't seem useful to have seq_cst at all then :)

Relevant: if you're acq or rel, you will have the `fence` bit set in Effects. In other words, B3 treats acq/rel fences to be ordered with respect to each other.

This means that you can get seq_cst by using acq for your loads and rel for your stores.

This also matches what ARM does, although apparently there's some new alternate version of acq that doesn't set the CPU's equivalent of Effects::fence. That's OK, because I don't think we would have a use for that instruction yet.
Comment 13 Filip Pizlo 2017-02-23 13:56:53 PST
Created attachment 302571 [details]
a bit more

Last version before I remove acq/rel flags and switch to relying exclusively on phantomRange.
Comment 14 Filip Pizlo 2017-02-23 16:01:23 PST
Created attachment 302599 [details]
more
Comment 15 Filip Pizlo 2017-02-24 10:48:03 PST
Looks like in order to do a really good job of this, we want to have a AtomicMemoryValue that has a Width arg.
Comment 16 Filip Pizlo 2017-02-24 20:53:49 PST
Created attachment 302731 [details]
a bit more
Comment 17 Filip Pizlo 2017-02-25 11:28:16 PST
Created attachment 302760 [details]
added the instructions to Air
Comment 18 Filip Pizlo 2017-02-25 12:01:08 PST
Created attachment 302761 [details]
added isel for fences
Comment 19 Filip Pizlo 2017-02-25 14:20:45 PST
Created attachment 302763 [details]
starting to work on isel for CAS

Turns out, to do it right on ARM, I need to allow the instruction selector to break basic blocks.  That seems super annoying.
Comment 20 Filip Pizlo 2017-02-26 09:17:54 PST
Created attachment 302786 [details]
built some tools I'll need for lowering CAS to LL/SC in isel
Comment 21 Filip Pizlo 2017-02-26 12:57:03 PST
Created attachment 302787 [details]
wrote some more isel

So much isel. So much fun!
Comment 22 Filip Pizlo 2017-02-26 16:22:05 PST
Created attachment 302795 [details]
more

finished CAS isel, I think
Comment 23 Filip Pizlo 2017-02-27 09:53:46 PST
Created attachment 302845 [details]
more

Making x86 CAS invertible
Comment 24 Filip Pizlo 2017-02-27 16:49:45 PST
Created attachment 302892 [details]
done with B3->Air isel for all atomics

so much isel, so much fun.
Comment 25 WebKit Commit Bot 2017-02-27 18:06:52 PST
Attachment 302892 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/B3HeapRange.h:91:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/b3/B3Width.h:75:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Width.h:75:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:95:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBlockInsertionSet.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Filip Pizlo 2017-02-27 20:17:25 PST
Created attachment 302917 [details]
adding X86 instruction formatting for atomics
Comment 27 Filip Pizlo 2017-02-27 21:46:41 PST
Created attachment 302922 [details]
more x86 atomic instruction formatting

all that CAS
Comment 28 Filip Pizlo 2017-02-28 09:35:07 PST
Created attachment 302941 [details]
so many x86 instructions

I think I finished the MacroAssembler for X86
Comment 29 Filip Pizlo 2017-02-28 10:48:03 PST
Created attachment 302949 [details]
just when I thought I was done with x86

So much x86
Comment 30 Filip Pizlo 2017-02-28 11:47:27 PST
Created attachment 302960 [details]
more x86

I love that you can express or64 with a 32-bit immediate on x86.

Maybe someone should do x86 instruction selection steganography. There are so many ways to waste bits.
Comment 31 Filip Pizlo 2017-02-28 12:08:11 PST
Created attachment 302963 [details]
it's starting to compile
Comment 32 Filip Pizlo 2017-02-28 12:37:23 PST
Created attachment 302968 [details]
more things compile

Filling in some gaps on x86
Comment 33 Filip Pizlo 2017-02-28 13:52:03 PST
Created attachment 302981 [details]
maybe the x86 part compiles now
Comment 34 Filip Pizlo 2017-02-28 17:28:54 PST
Created attachment 303015 [details]
it just passed some WeakCAS tests!
Comment 35 Filip Pizlo 2017-03-01 10:33:46 PST
Created attachment 303078 [details]
added tests for strong cas
Comment 36 Filip Pizlo 2017-03-01 13:37:10 PST
Created attachment 303109 [details]
added acq/rel tests
Comment 37 Filip Pizlo 2017-03-01 17:40:44 PST
Created attachment 303150 [details]
x86 looks solid

now I'll have to do the arms
Comment 38 Filip Pizlo 2017-03-01 20:52:47 PST
Created attachment 303170 [details]
rebased
Comment 39 Filip Pizlo 2017-03-01 21:40:16 PST
It looks like the ARM64 LL/SC instructions take an exotic kind of address which cannot be offset at all. That's cool. I'll just have to add a new notion of Addr to Air and the assemblers.
Comment 40 Filip Pizlo 2017-03-02 14:06:53 PST
Created attachment 303237 [details]
more ARM64
Comment 41 Filip Pizlo 2017-03-02 18:03:34 PST
Created attachment 303277 [details]
more

Boy, ARM64 isel for fancy loads/stores is tricky!
Comment 42 Filip Pizlo 2017-03-03 11:15:52 PST
Created attachment 303328 [details]
more
Comment 43 Filip Pizlo 2017-03-03 17:03:41 PST
Created attachment 303361 [details]
almost done arm wrestling

Note, this patch temporarily includes msaboff's disassembler patch.
Comment 44 Filip Pizlo 2017-03-03 18:00:27 PST
Created attachment 303369 [details]
passes all tests on arm

yay!
Comment 45 Filip Pizlo 2017-03-04 09:40:48 PST
Ha!  Even the Depend opcode works right!

On ARM, we lower to eor, but obfuscate it in both B3 and Air so that none of our optimization phases could ever realize that it returns zero.

testDepend32()...
Initial B3:
BB#0: ; frequency = 1.000000
    Int64 @0 = ArgumentReg(%r0)
    Int32 @1 = Load(@0, ControlDependent|Reads:Top)
    Int32 @2 = Depend(@1)
    Int64 @3 = ZExt32(@2)
    Int64 @4 = Add(@0, @3)
    Int32 @5 = Load(@4, offset = 4, ControlDependent|Reads:Top)
    Int32 @6 = Add(@1, @5)
    Void @7 = Return(@6, Terminal)
B3 after moveConstants, before generation:
BB#0: ; frequency = 1.000000
    Void @8 = Nop()
    Int64 @0 = ArgumentReg(%r0)
    Int32 @1 = Load(@0, ControlDependent|Reads:Top)
    Int32 @2 = Depend(@1)
    Int64 @3 = ZExt32(@2)
    Int64 @4 = Add(@0, @3)
    Int32 @5 = Load(@4, offset = 4, ControlDependent|Reads:Top)
    Int32 @6 = Add(@1, @5)
    Void @7 = Return(@6, Terminal)
Has Quirks: True
Initial air:
BB#0: ; frequency = 1.000000
    Move %r0, %tmp4, @0
    Move32 (%tmp4), %tmp2, @1
    Depend32 %tmp2, %tmp6, @2
    Move32 %tmp6, %tmp5, @3
    Add64 %tmp4, %tmp5, %tmp1, @4
    Move32 4(%tmp1), %tmp3, @5
    Add32 %tmp2, %tmp3, %tmp0, @6
    Move %tmp0, %r0, @7
    Ret32 %r0, @7
Air after optimizeBlockOrder, before generation:
Entrypoints: #0
BB#0: ; frequency = 1.000000
    Move32 (%r0), %r1, @1
    Depend32 %r1, %r2, @2
    Add64 %r0, %r2, %r0, @4
    Move32 4(%r0), %r0, @5
    Add32 %r1, %r0, %r0, @6
    Ret32 %r0, @7
Generated JIT code for B3::Compilation:
    Code at [0x3071cc000, 0x3071cc040):
         0x3071cc000:    stp    fp, lr, [sp, #-16]!
         0x3071cc004:    mov    fp, sp
         0x3071cc008:    ldur   w1, [x0]
         0x3071cc00c:    eor    w2, w1, w1
         0x3071cc010:    add    x0, x0, x2
         0x3071cc014:    ldur   w0, [x0, #4]
         0x3071cc018:    add    w0, w1, w0
         0x3071cc01c:    ldp    fp, lr, [sp], #16
         0x3071cc020:    ret    lr

And on x86, we lower it to a LoadFence, because those are free on x86. This unlocks awesome instruction selection over the Depend while still ensuring that the compiler obeys the constraint.

testDepend32()...
Initial B3:
BB#0: ; frequency = 1.000000
    Int64 @0 = ArgumentReg(%rdi)
    Int32 @1 = Load(@0, ControlDependent|Reads:Top)
    Int32 @2 = Depend(@1)
    Int64 @3 = ZExt32(@2)
    Int64 @4 = Add(@0, @3)
    Int32 @5 = Load(@4, offset = 4, ControlDependent|Reads:Top)
    Int32 @6 = Add(@1, @5)
    Void @7 = Return(@6, Terminal)
B3 after moveConstants, before generation:
BB#0: ; frequency = 1.000000
    Void @4 = Nop()
    Int64 @0 = ArgumentReg(%rdi)
    Int32 @1 = Load(@0, ControlDependent|Reads:Top)
    Void @8 = Fence(Fence|Writes:Top)
    Int32 @5 = Load(@0, offset = 4, ControlDependent|Reads:Top)
    Int32 @6 = Add(@1, @5)
    Void @7 = Return(@6, Terminal)
Has Quirks: True
Initial air:
BB#0: ; frequency = 1.000000
    Move %rdi, %tmp1, @0
    Move32 (%tmp1), %tmp2, @1
    LoadFence @8
    Move %tmp2, %tmp0, @6
    Add32 4(%tmp1), %tmp0, @6
    Move %tmp0, %rax, @7
    Ret32 %rax, @7
Air after optimizeBlockOrder, before generation:
Entrypoints: #0
BB#0: ; frequency = 1.000000
    Move32 (%rdi), %rax, @1
    LoadFence @8
    Add32 4(%rdi), %rax, @6
    Ret32 %rax, @7
Generated JIT code for B3::Compilation:
    Code at [0x2b7136801000, 0x2b7136801020):
      0x2b7136801000: push %rbp
      0x2b7136801001: mov %rsp, %rbp
      0x2b7136801004: mov (%rdi), %eax
      0x2b7136801006: add 0x4(%rdi), %eax
      0x2b7136801009: pop %rbp
      0x2b713680100a: ret 
testDepend32(): OK!
Comment 46 Filip Pizlo 2017-03-04 10:13:15 PST
Created attachment 303396 [details]
voila!

Hopefully it doesn't set the bots on fire.
Comment 47 WebKit Commit Bot 2017-03-04 10:16:14 PST
Attachment 303396 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/B3HeapRange.h:91:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3026:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3031:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3036:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3041:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3046:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3051:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3056:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3061:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3066:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3071:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3076:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3081:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3086:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3091:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3096:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3106:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3111:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Width.h:75:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Width.h:75:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:99:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:102:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:317:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:56:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:58:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1438:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1443:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1448:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1453:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1458:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1463:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBlockInsertionSet.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 37 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 48 Filip Pizlo 2017-03-04 10:20:28 PST
Created attachment 303397 [details]
more
Comment 49 WebKit Commit Bot 2017-03-04 10:23:36 PST
Attachment 303397 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/B3HeapRange.h:91:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3026:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3031:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3036:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3041:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3046:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3051:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3056:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3061:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3066:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3071:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3076:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3081:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3086:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3091:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3096:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3106:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3111:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Width.h:75:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Width.h:75:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:99:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:102:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:317:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:56:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:58:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1438:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1443:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1448:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1453:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1458:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1463:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBlockInsertionSet.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 37 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 50 Filip Pizlo 2017-03-04 13:29:46 PST
Created attachment 303405 [details]
fix some builds
Comment 51 WebKit Commit Bot 2017-03-04 13:32:48 PST
Attachment 303405 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/B3HeapRange.h:91:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3026:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3031:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3036:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3041:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3046:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3051:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3056:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3061:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3066:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3071:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3076:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3081:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3086:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3091:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3096:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3106:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3111:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Width.h:76:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Width.h:76:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:99:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:102:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:317:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:56:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:58:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1438:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1443:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1448:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1453:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1458:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1463:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBlockInsertionSet.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 37 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 Filip Pizlo 2017-03-04 19:52:42 PST
Created attachment 303435 [details]
more build fixes
Comment 53 WebKit Commit Bot 2017-03-04 19:55:40 PST
Attachment 303435 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/B3HeapRange.h:91:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3026:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3031:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3036:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3041:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3046:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3051:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3056:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3061:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3066:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3071:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3076:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3081:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3086:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3091:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3096:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3106:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3111:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Width.h:76:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Width.h:76:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:99:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:102:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:317:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:56:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:58:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1438:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1443:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1448:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1453:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1458:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1463:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBlockInsertionSet.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 37 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 54 Filip Pizlo 2017-03-04 20:28:50 PST
Created attachment 303437 [details]
try again to fix gtk build
Comment 55 WebKit Commit Bot 2017-03-04 20:32:26 PST
Attachment 303437 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/B3HeapRange.h:91:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3026:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3031:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3036:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3041:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3046:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3051:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3056:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3061:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3066:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3071:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3076:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3081:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3086:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3091:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3096:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3106:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3111:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Width.h:76:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Width.h:76:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:99:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:102:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:105:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:317:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:56:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:58:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1438:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1443:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1448:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1453:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1458:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1463:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:509:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBlockInsertionSet.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 37 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 56 JF Bastien 2017-03-05 03:46:57 PST
Comment on attachment 303437 [details]
try again to fix gtk build

View in context: https://bugs.webkit.org/attachment.cgi?id=303437&action=review

IIUC x86 cmpxchg never uses the flags that it sets, only its register? You can fold code such as this: old = cmpxchg(expected, desired) ; old ?= desired. The ZF flag is likely the most useful for == and !=, but others work too.



Otherwise, code looks good. I'm not familiar with everything :)

> Source/JavaScriptCore/ChangeLog:11
> +        - Atomic adds/sub/or/and/xor/xhg

"add"

> Source/JavaScriptCore/ChangeLog:44
> +        property in shared memory do not clobber non-atomics that ffect some other property in shared

"affect"

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:3387
> +        and32(TrustedImm32(0xff), dest);

I don't understand why the mask is necessary since the top bits are already zero (ditto below).

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:322
> +            m_assembler.incq_m(address.offset, address.base, address.index, address.scale);

I don't believe inc is faster than add because it only partially updates flags which can cause a stall. Agner concurs.

> Source/JavaScriptCore/b3/B3Effects.cpp:86
> +        || (fence && other.fence);

A few functions above, I think fences interfere with sideway exits because they're kindof like a delayed write. IIUC you can have write+fence+sideexit, and moving the fence after the exit can cause the write to occur in an unexpected order w.r.t. write in the side exit. I'm not sure their ordering ever matter?

Here, it looks like we never consider strictly equivalent effects? Say you have to actual fences (not fence effects, but actual barrier instructions), these don't interfere. I'm guessing this is rare, and we'd want to eliminate them separately?

> Source/JavaScriptCore/b3/B3Effects.h:96
> +        result.fence = true;

This isn't too conservative because we'd need to prove a call were "pure" or to entirely un-aliased heaps for any optimizations to be possible?

> Source/JavaScriptCore/b3/B3GenericBlockInsertionSet.h:69
> +        return insert(before->index(), frequency == frequency ? frequency : before->frequency());

isnan? :p

> Source/JavaScriptCore/b3/B3LowerMacros.cpp:82
> +        // This indicates that we've 

Unfinished.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:78
> +// OOPS!!! (need a bug URL)

Oops.

> Source/JavaScriptCore/b3/B3Opcode.h:220
> +    // AtomicWeakCAS(@exp, @new, @ptr) == Equal(AtomicStrongCAS(@exp, @new, @ptr), @exp)

I find the reverse equivalence useful: a looping weak CAS is equivalent to a single strong CAS.

> Source/JavaScriptCore/b3/B3Opcode.h:237
> +    // FIXME: Maybe we should have AtomicXchgNeg and AtomicXchgNot.

At some point, someone in LLVM decided that these read-modify-write patterns mattered:
Nand 	*p = ~(old & v)
Max 	*p = old >signed v ? old : v
Min 	*p = old <signed v ? old : v
UMax 	*p = old >unsigned v ? old : v
UMin 	*p = old <unsigned v ? old : v
I don't know that they do, but I assume there was some basis in reality.

> Source/JavaScriptCore/b3/B3Opcode.h:264
> +    // Except that the compiler never gets an opportunity to simplify out the BitXor.

Can dependencies be combined? I think that would be a possible future expansion of what you have here.

> Source/JavaScriptCore/b3/B3Value.cpp:610
> +            result.writes = memory->fenceRange();

I don't understand why loads write if they have a fence range. Same for stores below, and the read-modify-writes.
Comment 57 Filip Pizlo 2017-03-05 06:23:52 PST
(In reply to comment #56)
> Comment on attachment 303437 [details]
> try again to fix gtk build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303437&action=review
> 
> IIUC x86 cmpxchg never uses the flags that it sets, only its register? You
> can fold code such as this: old = cmpxchg(expected, desired) ; old ?=
> desired. The ZF flag is likely the most useful for == and !=, but others
> work too.

I dont understand what you're saying. If you read the code you'll see that we use the ZF flag from cmpzchg when it's profitable. 

> 
> 
> 
> Otherwise, code looks good. I'm not familiar with everything :)
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        - Atomic adds/sub/or/and/xor/xhg
> 
> "add"
> 
> > Source/JavaScriptCore/ChangeLog:44
> > +        property in shared memory do not clobber non-atomics that ffect some other property in shared
> 
> "affect"
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:3387
> > +        and32(TrustedImm32(0xff), dest);
> 
> I don't understand why the mask is necessary since the top bits are already
> zero (ditto below).

Because load acq on arm64 sign extends. 

> 
> > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:322
> > +            m_assembler.incq_m(address.offset, address.base, address.index, address.scale);
> 
> I don't believe inc is faster than add because it only partially updates
> flags which can cause a stall. Agner concurs.

Inc is smaller. 

Dont listen to this kind of performance advice. It's brain damage. These people who say these things haven't run an experiment in their whole lives. 

> 
> > Source/JavaScriptCore/b3/B3Effects.cpp:86
> > +        || (fence && other.fence);
> 
> A few functions above, I think fences interfere with sideway exits because
> they're kindof like a delayed write. IIUC you can have write+fence+sideexit,
> and moving the fence after the exit can cause the write to occur in an
> unexpected order w.r.t. write in the side exit. I'm not sure their ordering
> ever matter?
> 
> Here, it looks like we never consider strictly equivalent effects? Say you
> have to actual fences (not fence effects, but actual barrier instructions),
> these don't interfere. I'm guessing this is rare, and we'd want to eliminate
> them separately?

I think you don't understand what 'fence' does. It's just the serialization token for fences. It only has to interfere with itself. 

We indicate actual fencing behavior by using fence ranges. 

> 
> > Source/JavaScriptCore/b3/B3Effects.h:96
> > +        result.fence = true;
> 
> This isn't too conservative because we'd need to prove a call were "pure" or
> to entirely un-aliased heaps for any optimizations to be possible?

It is too conservative because the fence serialization token is only for serializing fences with each other. 

> 
> > Source/JavaScriptCore/b3/B3GenericBlockInsertionSet.h:69
> > +        return insert(before->index(), frequency == frequency ? frequency : before->frequency());
> 
> isnan? :p

I'm just moving code here. 

> 
> > Source/JavaScriptCore/b3/B3LowerMacros.cpp:82
> > +        // This indicates that we've 
> 
> Unfinished.
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:78
> > +// OOPS!!! (need a bug URL)
> 
> Oops.
> 
> > Source/JavaScriptCore/b3/B3Opcode.h:220
> > +    // AtomicWeakCAS(@exp, @new, @ptr) == Equal(AtomicStrongCAS(@exp, @new, @ptr), @exp)
> 
> I find the reverse equivalence useful: a looping weak CAS is equivalent to a
> single strong CAS.
> 
> > Source/JavaScriptCore/b3/B3Opcode.h:237
> > +    // FIXME: Maybe we should have AtomicXchgNeg and AtomicXchgNot.
> 
> At some point, someone in LLVM decided that these read-modify-write patterns
> mattered:
> Nand 	*p = ~(old & v)
> Max 	*p = old >signed v ? old : v
> Min 	*p = old <signed v ? old : v
> UMax 	*p = old >unsigned v ? old : v
> UMin 	*p = old <unsigned v ? old : v
> I don't know that they do, but I assume there was some basis in reality.

SAB doesn't use them and they have no basis in x86. 

> 
> > Source/JavaScriptCore/b3/B3Opcode.h:264
> > +    // Except that the compiler never gets an opportunity to simplify out the BitXor.
> 
> Can dependencies be combined? 

Yes. 

> I think that would be a possible future
> expansion of what you have here.
> 
> > Source/JavaScriptCore/b3/B3Value.cpp:610
> > +            result.writes = memory->fenceRange();
> 
> I don't understand why loads write if they have a fence range. Same for
> stores below, and the read-modify-writes.

Acquire is a phantom write and release is a phantom read. That's the B3 memory model.
Comment 58 Saam Barati 2017-03-05 10:16:09 PST
(In reply to comment #57)
> (In reply to comment #56)
> > Comment on attachment 303437 [details]
> > try again to fix gtk build
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=303437&action=review
> > 
> > IIUC x86 cmpxchg never uses the flags that it sets, only its register? You
> > can fold code such as this: old = cmpxchg(expected, desired) ; old ?=
> > desired. The ZF flag is likely the most useful for == and !=, but others
> > work too.
> 
> I dont understand what you're saying. If you read the code you'll see that
> we use the ZF flag from cmpzchg when it's profitable. 
> 
> > 
> > 
> > 
> > Otherwise, code looks good. I'm not familiar with everything :)
> > 
> > > Source/JavaScriptCore/ChangeLog:11
> > > +        - Atomic adds/sub/or/and/xor/xhg
> > 
> > "add"
> > 
> > > Source/JavaScriptCore/ChangeLog:44
> > > +        property in shared memory do not clobber non-atomics that ffect some other property in shared
> > 
> > "affect"
> > 
> > > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:3387
> > > +        and32(TrustedImm32(0xff), dest);
> > 
> > I don't understand why the mask is necessary since the top bits are already
> > zero (ditto below).
> 
> Because load acq on arm64 sign extends. 
> 
> > 
> > > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:322
> > > +            m_assembler.incq_m(address.offset, address.base, address.index, address.scale);
> > 
> > I don't believe inc is faster than add because it only partially updates
> > flags which can cause a stall. Agner concurs.
> 
> Inc is smaller. 
> 
> Dont listen to this kind of performance advice. It's brain damage. These
> people who say these things haven't run an experiment in their whole lives. 
We could very easily run this experiment inside JSC, too.

> 
> > 
> > > Source/JavaScriptCore/b3/B3Effects.cpp:86
> > > +        || (fence && other.fence);
> > 
> > A few functions above, I think fences interfere with sideway exits because
> > they're kindof like a delayed write. IIUC you can have write+fence+sideexit,
> > and moving the fence after the exit can cause the write to occur in an
> > unexpected order w.r.t. write in the side exit. I'm not sure their ordering
> > ever matter?
> > 
> > Here, it looks like we never consider strictly equivalent effects? Say you
> > have to actual fences (not fence effects, but actual barrier instructions),
> > these don't interfere. I'm guessing this is rare, and we'd want to eliminate
> > them separately?
> 
> I think you don't understand what 'fence' does. It's just the serialization
> token for fences. It only has to interfere with itself. 
> 
> We indicate actual fencing behavior by using fence ranges. 
> 
> > 
> > > Source/JavaScriptCore/b3/B3Effects.h:96
> > > +        result.fence = true;
> > 
> > This isn't too conservative because we'd need to prove a call were "pure" or
> > to entirely un-aliased heaps for any optimizations to be possible?
> 
> It is too conservative because the fence serialization token is only for
> serializing fences with each other. 
> 
> > 
> > > Source/JavaScriptCore/b3/B3GenericBlockInsertionSet.h:69
> > > +        return insert(before->index(), frequency == frequency ? frequency : before->frequency());
> > 
> > isnan? :p
> 
> I'm just moving code here. 
> 
> > 
> > > Source/JavaScriptCore/b3/B3LowerMacros.cpp:82
> > > +        // This indicates that we've 
> > 
> > Unfinished.
> > 
> > > Source/JavaScriptCore/b3/B3LowerToAir.cpp:78
> > > +// OOPS!!! (need a bug URL)
> > 
> > Oops.
> > 
> > > Source/JavaScriptCore/b3/B3Opcode.h:220
> > > +    // AtomicWeakCAS(@exp, @new, @ptr) == Equal(AtomicStrongCAS(@exp, @new, @ptr), @exp)
> > 
> > I find the reverse equivalence useful: a looping weak CAS is equivalent to a
> > single strong CAS.
> > 
> > > Source/JavaScriptCore/b3/B3Opcode.h:237
> > > +    // FIXME: Maybe we should have AtomicXchgNeg and AtomicXchgNot.
> > 
> > At some point, someone in LLVM decided that these read-modify-write patterns
> > mattered:
> > Nand 	*p = ~(old & v)
> > Max 	*p = old >signed v ? old : v
> > Min 	*p = old <signed v ? old : v
> > UMax 	*p = old >unsigned v ? old : v
> > UMin 	*p = old <unsigned v ? old : v
> > I don't know that they do, but I assume there was some basis in reality.
> 
> SAB doesn't use them and they have no basis in x86. 
> 
> > 
> > > Source/JavaScriptCore/b3/B3Opcode.h:264
> > > +    // Except that the compiler never gets an opportunity to simplify out the BitXor.
> > 
> > Can dependencies be combined? 
> 
> Yes. 
> 
> > I think that would be a possible future
> > expansion of what you have here.
> > 
> > > Source/JavaScriptCore/b3/B3Value.cpp:610
> > > +            result.writes = memory->fenceRange();
> > 
> > I don't understand why loads write if they have a fence range. Same for
> > stores below, and the read-modify-writes.
> 
> Acquire is a phantom write and release is a phantom read. That's the B3
> memory model.
Comment 59 Filip Pizlo 2017-03-05 11:44:00 PST
(In reply to comment #58)
> (In reply to comment #57)
> > (In reply to comment #56)
> > > Comment on attachment 303437 [details]
> > > try again to fix gtk build
> > > 
> > > > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:322
> > > > +            m_assembler.incq_m(address.offset, address.base, address.index, address.scale);
> > > 
> > > I don't believe inc is faster than add because it only partially updates
> > > flags which can cause a stall. Agner concurs.
> > 
> > Inc is smaller. 
> > 
> > Dont listen to this kind of performance advice. It's brain damage. These
> > people who say these things haven't run an experiment in their whole lives. 
> We could very easily run this experiment inside JSC, too.

We did!

https://trac.webkit.org/changeset/154158

If it had been a regression, we would have detected it at the time. I remember this change well because we had this exact conversation. It turns out that this change was a progression in the CSS JIT and it had no effect on the JSC JITs.  I believe that the progression was thanks to code size.

It's an easy test to rerun.  I think that we would need a definite win on a macrobenchmark suite (Octane, JetStream, or something like that) in order to remove our use of inc, since for big sites we want to err on the side of smaller code.
Comment 60 Saam Barati 2017-03-05 13:43:41 PST
(In reply to comment #59)
> (In reply to comment #58)
> > (In reply to comment #57)
> > > (In reply to comment #56)
> > > > Comment on attachment 303437 [details]
> > > > try again to fix gtk build
> > > > 
> > > > > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:322
> > > > > +            m_assembler.incq_m(address.offset, address.base, address.index, address.scale);
> > > > 
> > > > I don't believe inc is faster than add because it only partially updates
> > > > flags which can cause a stall. Agner concurs.
> > > 
> > > Inc is smaller. 
> > > 
> > > Dont listen to this kind of performance advice. It's brain damage. These
> > > people who say these things haven't run an experiment in their whole lives. 
> > We could very easily run this experiment inside JSC, too.
> 
> We did!
> 
> https://trac.webkit.org/changeset/154158
> 
> If it had been a regression, we would have detected it at the time. I
> remember this change well because we had this exact conversation. It turns
> out that this change was a progression in the CSS JIT and it had no effect
> on the JSC JITs.  I believe that the progression was thanks to code size.
Cool. This does not surprise me.

My original comment was mostly in favor of your stance of determining perf wins through experimentation and real data. If I heard both hypotheses in isolation, I would say each sounds interesting. 1, emit less code. 2, don't introduce stalls. But I am in total agreement it's paramount to actually measure these hypotheses before determining which is superior.

> It's an easy test to rerun.  I think that we would need a definite win on a
> macrobenchmark suite (Octane, JetStream, or something like that) in order to
> remove our use of inc, since for big sites we want to err on the side of
> smaller code.
Comment 61 JF Bastien 2017-03-06 11:34:20 PST
Cool, thanks for the answers! Besides the few comments I had, all looks good.

I'm not confident I understand how the fence ranges work, but I generally like that capability and would like it in C++: wg21.link/p0153
Comment 62 Filip Pizlo 2017-03-06 20:11:11 PST
Created attachment 303609 [details]
the patch

Rebased. Added bug links to all of the FIXMEs. Fixed some semantic slop in subwidth CASes.
Comment 63 WebKit Commit Bot 2017-03-06 20:13:15 PST
Attachment 303609 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/B3HeapRange.h:91:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3026:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3031:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3036:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3041:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3046:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3051:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3056:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3061:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3066:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3071:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3076:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3081:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3086:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3091:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3096:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3106:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3111:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Width.h:81:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Width.h:81:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:101:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:104:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:107:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:107:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:317:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:56:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:58:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1438:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1443:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1448:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1453:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1458:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1463:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:512:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:512:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBlockInsertionSet.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 37 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 64 Filip Pizlo 2017-03-09 09:42:36 PST
Created attachment 303928 [details]
the patch
Comment 65 WebKit Commit Bot 2017-03-09 09:44:28 PST
Attachment 303928 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/B3HeapRange.h:91:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3026:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3031:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3036:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3041:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3046:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3051:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3056:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3061:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3066:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3071:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3076:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3081:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3086:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3091:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3096:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3106:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3111:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Width.h:81:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Width.h:81:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:101:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:104:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:107:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:107:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:317:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:56:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:58:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1438:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1443:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1448:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1453:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1458:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:1463:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:512:  The parameter name "bank" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:512:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBlockInsertionSet.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 37 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 66 Keith Miller 2017-03-09 17:56:20 PST
Comment on attachment 303928 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303928&action=review

r=me.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:4140
> +    void atomicStrongCAS(StatusCondition cond, RegisterID expectedAndResult, RegisterID newValue, Address address, RegisterID result)

Side note: We should have a ClobberedRegisterID class so it's not possible to accidentally use a function that clobbers a register you don't expect.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:4142
> +        signExtend<datasize>(expectedAndResult, expectedAndResult);

I'm not sure I get this. why sign extend?

> Source/JavaScriptCore/b3/B3LowerMacros.cpp:82
> +        // This indicates that we've 

Looks like this indicates you lost your train of thought :P
Comment 67 Saam Barati 2017-03-09 20:10:13 PST
Comment on attachment 303928 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303928&action=review

> Source/JavaScriptCore/b3/B3LowerMacros.cpp:255
> +                // bits in the operand. . So, either way, we need to throw a sign-extend on these

double period.

> Source/JavaScriptCore/b3/B3LowerMacros.cpp:263
> +                        m_value->child(0) = m_insertionSet.insert<Value>(
> +                            m_index, Neg, m_origin, m_value->child(0));

What if child0 is INT_MIN?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2235
> +        storeBlock->append(storeCondOpcode(width, atomic->hasFence()), m_value, newValueTmp, address, successBoolResultTmp);

What does store conditional return when it successfully stores?
Comment 68 Filip Pizlo 2017-03-09 21:11:06 PST
(In reply to comment #67)
> Comment on attachment 303928 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303928&action=review
> 
> > Source/JavaScriptCore/b3/B3LowerMacros.cpp:255
> > +                // bits in the operand. . So, either way, we need to throw a sign-extend on these
> 
> double period.
> 
> > Source/JavaScriptCore/b3/B3LowerMacros.cpp:263
> > +                        m_value->child(0) = m_insertionSet.insert<Value>(
> > +                            m_index, Neg, m_origin, m_value->child(0));
> 
> What if child0 is INT_MIN?

Then add and sub behave the same way!

> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2235
> > +        storeBlock->append(storeCondOpcode(width, atomic->hasFence()), m_value, newValueTmp, address, successBoolResultTmp);
> 
> What does store conditional return when it successfully stores?

The ARM StoreCond returns 0 on success. So that's what Air does.
Comment 69 Filip Pizlo 2017-03-10 09:50:11 PST
Landed in https://trac.webkit.org/changeset/213714