Description
Filip Pizlo
2016-09-21 11:12:01 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. (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. 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. 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. Created attachment 302367 [details]
it begins
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. What would "no acq/rel" mean? seq_cst, or non-atomic? (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. 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. (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? (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 :) (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. Created attachment 302571 [details]
a bit more
Last version before I remove acq/rel flags and switch to relying exclusively on phantomRange.
Created attachment 302599 [details]
more
Looks like in order to do a really good job of this, we want to have a AtomicMemoryValue that has a Width arg. Created attachment 302731 [details]
a bit more
Created attachment 302760 [details]
added the instructions to Air
Created attachment 302761 [details]
added isel for fences
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.
Created attachment 302786 [details]
built some tools I'll need for lowering CAS to LL/SC in isel
Created attachment 302787 [details]
wrote some more isel
So much isel. So much fun!
Created attachment 302795 [details]
more
finished CAS isel, I think
Created attachment 302845 [details]
more
Making x86 CAS invertible
Created attachment 302892 [details]
done with B3->Air isel for all atomics
so much isel, so much fun.
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.
Created attachment 302917 [details]
adding X86 instruction formatting for atomics
Created attachment 302922 [details]
more x86 atomic instruction formatting
all that CAS
Created attachment 302941 [details]
so many x86 instructions
I think I finished the MacroAssembler for X86
Created attachment 302949 [details]
just when I thought I was done with x86
So much x86
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.
Created attachment 302963 [details]
it's starting to compile
Created attachment 302968 [details]
more things compile
Filling in some gaps on x86
Created attachment 302981 [details]
maybe the x86 part compiles now
Created attachment 303015 [details]
it just passed some WeakCAS tests!
Created attachment 303078 [details]
added tests for strong cas
Created attachment 303109 [details]
added acq/rel tests
Created attachment 303150 [details]
x86 looks solid
now I'll have to do the arms
Created attachment 303170 [details]
rebased
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. Created attachment 303237 [details]
more ARM64
Created attachment 303277 [details]
more
Boy, ARM64 isel for fancy loads/stores is tricky!
Created attachment 303328 [details]
more
Created attachment 303361 [details]
almost done arm wrestling
Note, this patch temporarily includes msaboff's disassembler patch.
Created attachment 303369 [details]
passes all tests on arm
yay!
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! Created attachment 303396 [details]
voila!
Hopefully it doesn't set the bots on fire.
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.
Created attachment 303397 [details]
more
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.
Created attachment 303405 [details]
fix some builds
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.
Created attachment 303435 [details]
more build fixes
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.
Created attachment 303437 [details]
try again to fix gtk build
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 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. (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. (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. (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. (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. 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 Created attachment 303609 [details]
the patch
Rebased. Added bug links to all of the FIXMEs. Fixed some semantic slop in subwidth CASes.
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.
Created attachment 303928 [details]
the patch
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 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 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? (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. Landed in https://trac.webkit.org/changeset/213714 |