Bug 162349 – B3 should have comprehensive support for atomic operations

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

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.

[JF Bastien]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 302367 [details]
it begins

[Filip Pizlo]

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.

[JF Bastien]

What would "no acq/rel" mean? seq_cst, or non-atomic?

[Filip Pizlo]

(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.

[JF Bastien]

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.

[Filip Pizlo]

(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?

[JF Bastien]

(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 :)

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 302571 [details]
a bit more

Last version before I remove acq/rel flags and switch to relying exclusively on phantomRange.

[Filip Pizlo]

Created attachment 302599 [details]
more

[Filip Pizlo]

Looks like in order to do a really good job of this, we want to have a AtomicMemoryValue that has a Width arg.

[Filip Pizlo]

Created attachment 302731 [details]
a bit more

[Filip Pizlo]

Created attachment 302760 [details]
added the instructions to Air

[Filip Pizlo]

Created attachment 302761 [details]
added isel for fences

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 302786 [details]
built some tools I'll need for lowering CAS to LL/SC in isel

[Filip Pizlo]

Created attachment 302787 [details]
wrote some more isel

So much isel. So much fun!

[Filip Pizlo]

Created attachment 302795 [details]
more

finished CAS isel, I think

[Filip Pizlo]

Created attachment 302845 [details]
more

Making x86 CAS invertible

[Filip Pizlo]

Created attachment 302892 [details]
done with B3->Air isel for all atomics

so much isel, so much fun.

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 302917 [details]
adding X86 instruction formatting for atomics

[Filip Pizlo]

Created attachment 302922 [details]
more x86 atomic instruction formatting

all that CAS

[Filip Pizlo]

Created attachment 302941 [details]
so many x86 instructions

I think I finished the MacroAssembler for X86

[Filip Pizlo]

Created attachment 302949 [details]
just when I thought I was done with x86

So much x86

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 302963 [details]
it's starting to compile

[Filip Pizlo]

Created attachment 302968 [details]
more things compile

Filling in some gaps on x86

[Filip Pizlo]

Created attachment 302981 [details]
maybe the x86 part compiles now

[Filip Pizlo]

Created attachment 303015 [details]
it just passed some WeakCAS tests!

[Filip Pizlo]

Created attachment 303078 [details]
added tests for strong cas

[Filip Pizlo]

Created attachment 303109 [details]
added acq/rel tests

[Filip Pizlo]

Created attachment 303150 [details]
x86 looks solid

now I'll have to do the arms

[Filip Pizlo]

Created attachment 303170 [details]
rebased

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 303237 [details]
more ARM64

[Filip Pizlo]

Created attachment 303277 [details]
more

Boy, ARM64 isel for fancy loads/stores is tricky!

[Filip Pizlo]

Created attachment 303328 [details]
more

[Filip Pizlo]

Created attachment 303361 [details]
almost done arm wrestling

Note, this patch temporarily includes msaboff's disassembler patch.

[Filip Pizlo]

Created attachment 303369 [details]
passes all tests on arm

yay!

[Filip Pizlo]

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!

[Filip Pizlo]

Created attachment 303396 [details]
voila!

Hopefully it doesn't set the bots on fire.

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 303397 [details]
more

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 303405 [details]
fix some builds

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 303435 [details]
more build fixes

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 303437 [details]
try again to fix gtk build

[WebKit Commit Bot]

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.

[JF Bastien]

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.

[Filip Pizlo]

(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.

[Saam Barati]

(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.

[Filip Pizlo]

(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.

[Saam Barati]

(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.

[JF Bastien]

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

[Filip Pizlo]

Created attachment 303609 [details]
the patch

Rebased. Added bug links to all of the FIXMEs. Fixed some semantic slop in subwidth CASes.

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 303928 [details]
the patch

[WebKit Commit Bot]

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.

[Keith Miller]

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

[Saam Barati]

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?

[Filip Pizlo]

(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.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/213714