[+] auto-unroll - #8
Conversation
let -O3 auto-inline both loops in a smarter way.
I think you are saying that the compiler under some setting unrolls better than manually unrolled code. I do not think that you are saying that this new code with C macros... #define z(i) base_ptr[base + i] = static_cast<uint32_t>(idx) + trailingzeroes(bits); bits = bits & (bits - 1);
really_inline void flatten_bits(uint32_t *base_ptr, uint32_t &base,
uint32_t idx, uint64_t bits) {
if (bits != 0u) {
uint32_t cnt = hamming(bits);
uint32_t next_base = base + cnt
z(0)z(1)z(2)z(3)z(4)z(5)z(6)z(7)... will be significantly better than the existing code... really_inline void flatten_bits(uint32_t *base_ptr, uint32_t &base,
uint32_t idx, uint64_t bits) {
if (bits != 0u) {
uint32_t cnt = hamming(bits);
uint32_t next_base = base + cnt;
base_ptr[base + 0] = static_cast<uint32_t>(idx) + trailingzeroes(bits);
bits = bits & (bits - 1);
base_ptr[base + 1] = static_cast<uint32_t>(idx) + trailingzeroes(bits);
bits = bits & (bits - 1);
base_ptr[base + 2] = static_cast<uint32_t>(idx) + trailingzeroes(bits);
bits = bits & (bits - 1);
base_ptr[base + 3] = static_cast<uint32_t>(idx) + trailingzeroes(bits);
bits = bits & (bits - 1);
base_ptr[base + 4] = static_cast<uint32_t>(idx) + trailingzeroes(bits);
bits = bits & (bits - 1);
base_ptr[base + 5] = static_cast<uint32_t>(idx) + trailingzeroes(bits);
bits = bits & (bits - 1);
base_ptr[base + 6] = static_cast<uint32_t>(idx) + trailingzeroes(bits);
bits = bits & (bits - 1);
base_ptr[base + 7] = static_cast<uint32_t>(idx) + trailingzeroes(bits);
bits = bits & (bits - 1);If not, why introduce a macro ? Macros tend to harm readability and maintainability as you know. Effectively, you are saying that you are getting better results without the manual unrolling. That's interesting. |
|
@kelas The Here is what I get... (Apple ARM, clang 12) And with GCC 8 under Zen 2... As you can see, I get nothing close to a 30% performance gain when avoiding the manual unrolling. In fact, I get exactly the same results down to the instruction count. |
|
BTW here is the current code in simdjson (the basis for simdcsv): if (bits == 0)
return;
int cnt = static_cast<int>(count_ones(bits));
// Do the first 8 all together
for (int i=0; i<8; i++) {
this->tail[i] = idx + trailing_zeroes(bits);
bits = clear_lowest_bit(bits);
}
// Do the next 8 all together (we hope in most cases it won't happen at all
// and the branch is easily predicted).
if (simdjson_unlikely(cnt > 8)) {
for (int i=8; i<16; i++) {
this->tail[i] = idx + trailing_zeroes(bits);
bits = clear_lowest_bit(bits);
}
// Most files don't have 16+ structurals per block, so we take several basically guaranteed
// branch mispredictions here. 16+ structurals per block means either punctuation ({} [] , :)
// or the start of a value ("abc" true 123) every four characters.
if (simdjson_unlikely(cnt > 16)) {
int i = 16;
do {
this->tail[i] = idx + trailing_zeroes(bits);
bits = clear_lowest_bit(bits);
i++;
} while (i < cnt);
}
}
this->tail += cnt;
}
};
|
|
Macros tend to harm readability and maintainability as you know.
depends on your choice of macros :) this particular style indeed doesn't match the neighbourhood, it's a lazy copypaste from the C port, which is 20 lines of code total. i have no problem rewriting those two loops the loopy way, will do that later.
Effectively, you are saying that you are getting better results without the manual unrolling. That's interesting.
correct, i believe that's what i'm saying.
gcc8
sorry, i don't have access to legacy compiler tech. compilers do get better!
30% performance gain when avoiding the manual unrolling
affirmative. if you saw the disasm listings included in PR, the gist of it is this:
1. clang12 and gcc10 auto-unroll each 8-step loop to this:
```
1000044c4: 41 89 c2 movl %eax, %r10d
1000044c7: 31 f6 xorl %esi, %esi
1000044c9: f3 48 0f bc f1 tzcntq %rcx, %rsi
1000044ce: 01 d6 addl %edx, %esi
1000044d0: 42 89 34 97 movl %esi, (%rdi,%r10,4)
1000044d4: c4 e2 f0 f3 c9 blsrq %rcx, %rcx
1000044d9: 31 f6 xorl %esi, %esi
1000044db: f3 48 0f bc f1 tzcntq %rcx, %rsi
1000044e0: 01 d6 addl %edx, %esi
1000044e2: 42 89 74 97 04 movl %esi, 4(%rdi,%r10,4)
1000044e7: c4 e2 f0 f3 c9 blsrq %rcx, %rcx
1000044ec: 31 f6 xorl %esi, %esi
1000044ee: f3 48 0f bc f1 tzcntq %rcx, %rsi
1000044f3: 01 d6 addl %edx, %esi
1000044f5: 42 89 74 97 08 movl %esi, 8(%rdi,%r10,4)
1000044fa: c4 e2 f0 f3 c9 blsrq %rcx, %rcx
1000044ff: 31 f6 xorl %esi, %esi
100004501: f3 48 0f bc f1 tzcntq %rcx, %rsi
100004506: 01 d6 addl %edx, %esi
100004508: 42 89 74 97 0c movl %esi, 12(%rdi,%r10,4)
```
2. whereas manual unroll heroics compile to this:
```
1000044c2: 44 89 14 b7 movl %r10d, (%rdi,%rsi,4)
1000044c6: c4 e2 a8 f3 c9 blsrq %rcx, %r10
1000044cb: 31 f6 xorl %esi, %esi
1000044cd: f3 49 0f bc f2 tzcntq %r10, %rsi
1000044d2: 01 d6 addl %edx, %esi
1000044d4: 8d 48 01 leal 1(%rax), %ecx //!< here be dragons
1000044d7: 89 34 8f movl %esi, (%rdi,%rcx,4)
1000044da: c4 c2 a8 f3 ca blsrq %r10, %r10
1000044df: 31 f6 xorl %esi, %esi
1000044e1: f3 49 0f bc f2 tzcntq %r10, %rsi
1000044e6: 01 d6 addl %edx, %esi
1000044e8: 8d 48 02 leal 2(%rax), %ecx //!< more here
1000044eb: 89 34 8f movl %esi, (%rdi,%rcx,4)
1000044ee: c4 c2 a8 f3 ca blsrq %r10, %r10
1000044f3: 31 f6 xorl %esi, %esi
1000044f5: f3 49 0f bc f2 tzcntq %r10, %rsi
1000044fa: 01 d6 addl %edx, %esi
1000044fc: 8d 48 03 leal 3(%rax), %ecx //!< ad infinitum
```
i'm not sure where exactly the useless `lea` comes from, and tbh i don't want to know - sometimes it's ok to let the compiler do the right thing.
hope this helps
k.
…Sent from my Imperial Star Destroyer
|
|
yup from the looks of it simdjson version would get auto-unrolled like a boss. blue skies.
but yeah - i think it is the first time i've hit a major performance degradation from an otherwise perfectly braindead manually unrolled loop.
them compiler writers know their thing, eh.
…On Fri, 16 Apr 2021 at 17:51, Daniel Lemire ***@***.***> wrote:
BTW here is the current code in simdjson (the basis for simdcsv):
if (bits == 0)
return;
int cnt = static_cast<int>(count_ones(bits));
// Do the first 8 all together
for (int i=0; i<8; i++) {
this->tail[i] = idx + trailing_zeroes(bits);
bits = clear_lowest_bit(bits);
}
// Do the next 8 all together (we hope in most cases it won't happen at all
// and the branch is easily predicted).
if (simdjson_unlikely(cnt > 8)) {
for (int i=8; i<16; i++) {
this->tail[i] = idx + trailing_zeroes(bits);
bits = clear_lowest_bit(bits);
}
// Most files don't have 16+ structurals per block, so we take several basically guaranteed
// branch mispredictions here. 16+ structurals per block means either punctuation ({} [] , :)
// or the start of a value ("abc" true 123) every four characters.
if (simdjson_unlikely(cnt > 16)) {
int i = 16;
do {
this->tail[i] = idx + trailing_zeroes(bits);
bits = clear_lowest_bit(bits);
i++;
} while (i < cnt);
}
}
this->tail += cnt;
}
};
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALVXXV7T7LE7F5E56J5D73TJBMIXANCNFSM43A76SYQ>
.
--
k.
--
Sent from my Imperial Star Destroyer
|
If you follow... You will notice that I offer Docker configurations and scripts allowing you to run it with the compilers I had used back in 2019. Numbers with GCC 10 and Zen 2: Numbers with Zen 2 and LLVM clang 9: GCC 10 is current. GCC 8 came out in 2018.
I submit to you that if the running time is 30% higher then you have discovered a bug with LLVM and you should report it to LLVM. |
|
Here my results with LLVM clang 13 on Zen 2: So I get the same results (exactly same number of instructions) with both functions. You can use |
@lemire - do you mean this Dockerfile.llvmlatest? |
Yes. |
|
It appears that @kelas rediscovered an optimization that has been applied to simdjson a few years ago. I still find it puzzling that an auto-unrolled loop results in a better code than its repeated body. |
it actually seems to fluctuate between 10% and 25%, i guess depending on the cost of
this is a bold statement, Daniel. both gcc10 and clang12 (-O3) auto-unroll these two loops without extra crud, and work faster. this is a double-blind validated result. sorry. i therefore submit to you that this day and age manual loop wizardry from the language userland needs to be carefully profiled and validated. In this particular case, two pages of copy-pasted (also results in style which looks a bit watery to some, but tastes differ. some people hate macros.) it's also reassuring to hear that simdjson was relieved of this optimisation at some point. great project by the way, keep up the good work. k. |
|
According to @jkeiser, de-unrolling did not affect performance and was applied for purely aesthetic reasons. See the original Simplify and optimize flatten_bits patch. |
thanks for digging that out, alex! i second that, i originally de-unrolled both loops in the C port just because i didn't want to have zzzzzzzz in my code, but then i profiled it, and i was in for a little surprise. |
|
Most compilers at the highest level of optimization will unroll short simple loops... short is about 16 iterations. Note that this is sensitive to the optimization flags. If you compile with -Os or -O2, then you will not get unrolling (GCC will not unroll at all at -O2). Both the -Os or -O2 flags are fairly popular and conventional. If you really want unrolling, you better do it yourself in the code. Note here that if you look at the code, even with the short loops from 0 to 8, it is still effectively unrolled. The no-unrolling would be like so... do {
this->tail[i] = idx + trailing_zeroes(bits);
bits = clear_lowest_bit(bits);
i++;
} while (i < cnt);Empirically, it proves beneficial within simdjson to do the song and dance that you see.
I do not recall it this way. It was not an optimization. Rather the author of this contribution noticed that when building release builds with CMake, we got the needed unrolling. There was never any claim that it improved the performance. |
this little hiccup at hand provides ample evidence that it just ain't so. a human getting in the way of a modern optimising C/C++ compiler has every chance to end up being the bottleneck.
still the jumbo commit description claims 15% improvement of stage 1 code, which i trust is some kind of early tokenization pass. hard to know what transpired, and what compilers were at play.
there is one now. |
I disagree. I carefully provided multiple reproducible tests, using multiple compilers and more than one processor, using CPU performance counters, showing no difference. I am getting perfectly comparable binaries (as measured by performance counters) and the assembly outputs are what I am expecting. You are reporting that both GNU GCC and LLVM clang produce anomalous code that harms the performance. I submit to you that this should be reported. Here is the clang 12 assembly produced with the unrolled code: flatten_bits(unsigned int*, unsigned int&, unsigned int, unsigned long): # @flatten_bits(unsigned int*, unsigned int&, unsigned int, unsigned long)
test rcx, rcx
je .LBB0_6
popcnt r9, rcx
mov r10d, dword ptr [rsi]
lea r8d, [r10 + r9]
tzcnt rax, rcx
add eax, edx
mov dword ptr [rdi + 4*r10], eax
blsr r10, rcx
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
inc eax
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 2
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 3
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 4
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 5
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 6
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor eax, eax
tzcnt rax, r10
add eax, edx
mov ecx, dword ptr [rsi]
add ecx, 7
mov dword ptr [rdi + 4*rcx], eax
cmp r9d, 9
jb .LBB0_5
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 8
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 9
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 10
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 11
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 12
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 13
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 14
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor eax, eax
tzcnt rax, r10
add eax, edx
mov ecx, dword ptr [rsi]
add ecx, 15
mov dword ptr [rdi + 4*rcx], eax
cmp r9d, 17
jb .LBB0_5
blsr rax, r10
mov ecx, dword ptr [rsi]
add ecx, 16
mov dword ptr [rsi], ecx
.LBB0_4: # =>This Inner Loop Header: Depth=1
xor r9d, r9d
tzcnt r9, rax
add r9d, edx
mov ecx, ecx
mov dword ptr [rdi + 4*rcx], r9d
mov ecx, dword ptr [rsi]
inc ecx
blsr rax, rax
mov dword ptr [rsi], ecx
jne .LBB0_4
.LBB0_5:
mov dword ptr [rsi], r8d
.LBB0_6:
retThe core sequence is something like this... tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 8
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecxHere is the GCC 10 code: flatten_bits(unsigned int*, unsigned int&, unsigned int, unsigned long):
test rcx, rcx
je .L12
xor r10d, r10d
mov eax, DWORD PTR [rsi]
xor r8d, r8d
tzcnt r10, rcx
popcnt r8, rcx
add r10d, edx
lea r9d, [rax+r8]
mov DWORD PTR [rdi+rax*4], r10d
blsr rax, rcx
mov r10d, DWORD PTR [rsi]
xor ecx, ecx
tzcnt rcx, rax
blsr rax, rax
inc r10d
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 2
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 3
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 4
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 5
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 6
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
add r10d, 7
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
cmp r8d, 8
jle .L4
mov r10d, DWORD PTR [rsi]
xor ecx, ecx
blsr rax, rax
tzcnt rcx, rax
blsr rax, rax
add r10d, 8
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 9
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 10
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 11
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 12
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 13
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 14
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 15
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
cmp r8d, 16
jle .L4
mov ecx, DWORD PTR [rsi]
add ecx, 16
mov DWORD PTR [rsi], ecx
.L5:
xor r8d, r8d
mov ecx, ecx
tzcnt r8, rax
add r8d, edx
mov DWORD PTR [rdi+rcx*4], r8d
mov ecx, DWORD PTR [rsi]
inc ecx
blsr rax, rax
mov DWORD PTR [rsi], ecx
jne .L5
.L4:
mov DWORD PTR [rsi], r9d
ret
.L12:
ret
Both compiled with Link: https://godbolt.org/z/5sn1WEMen Now here is the 'rolled version' (same compiler setting). Clang. flatten_bits(unsigned int*, unsigned int&, unsigned int, unsigned long): # @flatten_bits(unsigned int*, unsigned int&, unsigned int, unsigned long)
test rcx, rcx
je .LBB0_6
popcnt r8, rcx
mov r9d, dword ptr [rsi]
tzcnt rax, rcx
add eax, edx
mov dword ptr [rdi + 4*r9], eax
blsr r10, rcx
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
inc eax
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 2
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 3
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 4
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 5
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 6
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor eax, eax
tzcnt rax, r10
add eax, edx
mov ecx, dword ptr [rsi]
add ecx, 7
mov dword ptr [rdi + 4*rcx], eax
add r9d, r8d
cmp r8d, 9
jb .LBB0_5
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 8
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 9
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 10
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 11
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 12
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 13
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 14
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecx
tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 15
mov dword ptr [rdi + 4*rax], ecx
cmp r8d, 17
jb .LBB0_5
blsr rax, r10
mov ecx, dword ptr [rsi]
add ecx, 16
mov dword ptr [rsi], ecx
.LBB0_4: # =>This Inner Loop Header: Depth=1
xor r8d, r8d
tzcnt r8, rax
add r8d, edx
mov ecx, ecx
mov dword ptr [rdi + 4*rcx], r8d
mov ecx, dword ptr [rsi]
inc ecx
blsr rax, rax
mov dword ptr [rsi], ecx
jne .LBB0_4
.LBB0_5:
mov dword ptr [rsi], r9d
.LBB0_6:
retThe core sequence is tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
inc eax
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecxIt is distinct from what we got in the rolled case, but barely, recall what we add... tzcnt rcx, r10
add ecx, edx
mov eax, dword ptr [rsi]
add eax, 8
mov dword ptr [rdi + 4*rax], ecx
blsr r10, r10
xor ecx, ecxImportantly, the instruction count is the same. And then GCC flatten_bits(unsigned int*, unsigned int&, unsigned int, unsigned long):
test rcx, rcx
je .L12
xor r10d, r10d
mov eax, DWORD PTR [rsi]
xor r8d, r8d
tzcnt r10, rcx
popcnt r8, rcx
add r10d, edx
lea r9d, [rax+r8]
mov DWORD PTR [rdi+rax*4], r10d
blsr rax, rcx
mov r10d, DWORD PTR [rsi]
xor ecx, ecx
tzcnt rcx, rax
blsr rax, rax
inc r10d
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 2
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 3
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 4
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 5
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 6
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 7
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
cmp r8d, 8
jle .L4
mov r10d, DWORD PTR [rsi]
xor ecx, ecx
tzcnt rcx, rax
blsr rax, rax
add r10d, 8
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 9
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 10
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 11
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 12
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 13
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
add r10d, 14
add ecx, edx
blsr rax, rax
mov DWORD PTR [rdi+r10*4], ecx
xor ecx, ecx
mov r10d, DWORD PTR [rsi]
tzcnt rcx, rax
blsr rax, rax
add r10d, 15
add ecx, edx
mov DWORD PTR [rdi+r10*4], ecx
cmp r8d, 16
jle .L4
mov ecx, DWORD PTR [rsi]
add ecx, 16
mov DWORD PTR [rsi], ecx
.L5:
xor r8d, r8d
mov ecx, ecx
tzcnt r8, rax
add r8d, edx
mov DWORD PTR [rdi+rcx*4], r8d
mov ecx, DWORD PTR [rsi]
inc ecx
blsr rax, rax
mov DWORD PTR [rsi], ecx
jne .L5
.L4:
mov DWORD PTR [rsi], r9d
ret
.L12:
retIf you switch to -O2 with GCC and the rolled code, you get this: flatten_bits(unsigned int*, unsigned int&, unsigned int, unsigned long):
mov r8d, edx
test rcx, rcx
je .L16
mov r9d, DWORD PTR [rsi]
xor r11d, r11d
xor eax, eax
popcnt r11, rcx
lea r10d, [r9+r11]
jmp .L4
.L18:
mov r9d, DWORD PTR [rsi]
.L4:
xor edx, edx
add r9d, eax
inc eax
tzcnt rdx, rcx
blsr rcx, rcx
add edx, r8d
mov DWORD PTR [rdi+r9*4], edx
cmp eax, 8
jne .L18
cmp r11d, 8
jle .L6
.L5:
xor edx, edx
mov r9d, eax
inc eax
add r9d, DWORD PTR [rsi]
tzcnt rdx, rcx
blsr rcx, rcx
add edx, r8d
mov DWORD PTR [rdi+r9*4], edx
cmp eax, 16
jne .L5
cmp r11d, 16
jle .L6
mov eax, DWORD PTR [rsi]
add eax, 16
mov DWORD PTR [rsi], eax
.L7:
xor edx, edx
mov eax, eax
tzcnt rdx, rcx
add edx, r8d
mov DWORD PTR [rdi+rax*4], edx
mov eax, DWORD PTR [rsi]
inc eax
blsr rcx, rcx
mov DWORD PTR [rsi], eax
jne .L7
.L6:
mov DWORD PTR [rsi], r10d
ret
.L16:
ret |
Yes. You are making this claim. I find the evidence lacking. It does not mean that your claim is not interesting. I'd love to document such a case myself. I love surprising things. Can you provide me with a reproducible test case, instrumented with instruction counts? Because here you are claiming that one function generates far more instructions than the other. These things are easy to quantify very precisely. The only source of noise in these cases has to do with the branch predictor. But the function is written to have relatively few branch. Just make sure that your input has predictable number of 1s per word and you will tame the predictions. Other than that, the instruction count can be known exactly with no room for variations. We can just count. There is no room for debate here. Provide the reproducible test case. You can get to pick your compiler : anything that runs under Docker is fair game. |
|
Here is a demonstration based on @kelas's code of clang seemingly inserting an extra instruction in manually unrolled code: https://godbolt.org/z/r75P1GzM8 Unroll: bsf r10, rcx
add r10d, edx
mov esi, eax ; <------ here
mov dword ptr [rdi + 4*rsi], r10d
lea rsi, [rcx - 1]
and rsi, rcxLoop: bsf rsi, rcx
add esi, edx
mov dword ptr [rdi + 4*r9], esi
lea rsi, [rcx - 1]
and rsi, rcxNote that I see an extra |
|
Here is an even simpler demo with an extra |
|
@abalkin The simdcsv library will not build on anything but NEON or AVX2 targets. Please compile either for NEON or AVX2 targets (e.g., add Even if you do so (change the target), you will still see the extra LEA. But here is what the current simdcsv code compiles to... https://godbolt.org/z/hjf8cTqGb There is no LEA as you can see. |
ok
here's the bare-minimum example of the exact |
|
Adding https://godbolt.org/z/PEshGKGes But as far as I know, @kelas did not see the extra |
|
@kelas - this is an interesting discussion, but it doesn't seem to be pertinate to this PR. I have compiled $ ls -l simdcsv*
-rwxr-xr-x 1 a staff 54320 Apr 16 19:36 simdcsv
-rwxr-xr-x 1 a staff 54320 Apr 16 19:35 simdcsv.a9494aNevertheless, I do see some increase in throughput: $ ./simdcsv -i1000 examples/nfl.csv
GB/s: 6.38095
$ ./simdcsv.a9494a -i1000 examples/nfl.csv
GB/s: 5.93843I have not tried to instrument these binaries as @lemire suggested or investigate the phenomenon any further, but apparently what you see in your code is not what makes this PR improve throughput in my environment. |
coincidence? :) this is getting a bit C++ voodoo, I'm not an expert, not my area, not very fascinating tbh. what i know for sure is that my C port is faster than this implementation, objsize you're welcome to check on your own, but don't hold your breath - C is C. |
|
What happens if you change SIMDCSV_BUFFERSIZE ? Note how it was doubled in this PR. I remember setting SIMDCSV_BUFFERSIZE "approximately". Regarding the LEA, we can sum it up as follows. If you look at the code, we are comparing... mov DWORD PTR [rdi+12+r8*4], ecxwith lea ecx, [rsi + 3]
mov dword ptr [rdi + 4*rcx], r9dIt is interesting. |
|
I am querying twitter to see if someone has an idea |
Does not seem to make a difference, but I now noticed that even with $ for i in `seq 10`; do ./simdcsv -i1000 examples/nfl.csv; done
GB/s: 6.26033
GB/s: 6.62941
GB/s: 5.83218
GB/s: 5.93164
GB/s: 6.43911
GB/s: 6.43891
GB/s: 6.13735
GB/s: 6.32976
GB/s: 6.25318
GB/s: 6.60594and the improvement (if any) is not as evident as I thought: $ for i in `seq 10`; do ./simdcsv.a9494a -i1000 examples/nfl.csv; done
GB/s: 6.8027
GB/s: 6.81799
GB/s: 6.03031
GB/s: 6.51562
GB/s: 6.20322
GB/s: 6.39817
GB/s: 5.96236
GB/s: 6.84384
GB/s: 6.88059
GB/s: 6.86554 |
|
Here are my results on a Zen 2 machine with GNU GCC 10. Main branch: This PR So at least in my test, this PR is not beneficial. Note that I am using a machine configured for testing (not a laptop). |
yes, back in 2019 you identified sweet spot to be four buffering interations for better instr/sec, i bumped it to 8 (also empirically, seems to perform marginally better on iron i have)
true! i think i see what it is. in my last snippet, the loopy version can be degraded to that said, profiling this kind of critical codepaths is essential in any event - only the less code there is to shovel around, the easier it gets to identify a potential imperfection. |
I am not sure that the |
Aha! This is clearly the issue. The unrolled version can similarly be "fixed" by changing ints to longs as in: z(0L)z(1L)z(2L)z(3L)z(4L)z(5L)z(6L)z(7L) |
|
@abalkin So Travis Downs on twitter pointed it out... https://twitter.com/trav_downs/status/1383213901685293059 If you make your index types match, you get matching outputs... |
So all three of us have discovered the same thing. Only I also pointed out that @kelas made his unrolled code worse than his loop which does not happen here. |
Well, in my case I had help from Travis Downs (twitter), but I still can't understand how this shows up in the PR. |
|
@lemire @abalkin gents, getting late where i am in the world. there's definitely a lesson to be learned here! curiously, the C implementation simply had no chance to blow up on loopctr/index type disparity, because it hardly ever uses explicit loops at all. a #define N(n,a...) {unsigned long long _n=(n),i=0;while(i<_n){a;++i;}}
main(){
char*s="simdjson";
N(strlen(s),putchar(s[i]))
}
//:~imho consistent application of a handful of such defines makes the coding experience a lot more fail-safe, contrary to @lemire's take on the famous subject of cpp abuse. |
that's why we have a pragma for that. |
I suspect it does not. The timings that you and I see are inconclusive and certainly don't support the claim of 30% improvement. Given the number of changes in this PR, even if speed-up was evident, it would be hard to attribute it to any specific change. My take away from this is that in the last two years or so, this code base fell behind simdjson on which it is bases and could benefit from borrowing some of the changes that happened there. |
That's absolutely correct. Even the front-end could be redone with something akin to on demand. We are first going to get simdjson to version 1.0. It is a matter of months at this time. Then I might jump on simdcsv. If some people want to help, they should reach out. |
|
yeah, 30% for C++ patch was clearly an undercooked conclusion - let me go ahead and fix that.
this PoC is a great starting point, and there exists a use case for the C implementation (aka "soup of 4 macros"), so hopefully we'd be able to offer some notes to compare once you get to CSV. closing on the subject of code soup and other forms of bloat, C version does indeed compile to approximately 10 extra kb compared to C++ found in this repo - only it packs a C port of @lemire's |
|
Thanks for sharing your work. I am not under the impression that anyone was concerned about bloat related to the C language. Look... I am loading web pages that span 2MB or more all of the time. |
|
Here is the size of the current page:
|
|
4MB. |
Speaking of helping out, wouldn't it be better to add CSV parsing support to simdjson rather than catching up in this codebase? I see that you suggested exactly that not so long ago. |
That's a possibility. Yes. There is a trade-off in terms of code complexity. The simdjson library is not for everyone. It is getting to be quite large. I can navigate it, but I doubt many people can. Adding CSV to it would leave even fewer people able to comprehend what is happening. I am not ruling anything out... but it is not just a matter of "let us add CSV support". |
|
my two pennies: CSV is better be lean and separate. two-stage designs seem to prevail in the wild, i believe for a good reason - stage 1 is generic enough (which is this repo, give or take — streaming/buffering/fifo can come too perhaps). but field parser machinery clearly wants a clean and efficient API for pluggable parsers. there's hardly a one-size-fits-all in CSV land. any views on that? |
OK, I'm somewhat of an absentee landlord here - I started simdcsv but ran out of steam (too many side projects and more recently, too much 'day job' work). So take my opinions with a grain of salt. I don't think it makes that much sense to add simdcsv to simdjson - unless simdjson is viewed as a "umbrella" project that has a lot of factored-out infrastructure for these kinds of projects. simdcsv shares some things with simdjson, but it doesn't really need the second stage of simdjson (or the new on-demand stuff) - CSV is pretty much a straightforward lexing job. There's no analogous parsing stage necessary (at least as I understood things). Glad to see people working on this. |
@lemire thanks for the pointer btw, that M1 profiler is really handy. |
4 participants
@geofflangdale @lemire
great stuff, guys! too bad the project went stale.
here's what i got:
flatten_bits()which generate suboptimal code (clang -O3 auto-unrolls better, tested on both intel and arm, a noticeable improvement)for more discussion regarding #1, see my C port https://github.com/kelas/ld with disassembly listings:
$ cd ld && make unroll nounrollor just:
https://github.com/kelas/ld/blob/master/zip.UNROLL
https://github.com/kelas/ld/blob/master/zip.NOUNROLL
PS. I just discovered that @lemire also attempted to make
flatten_bits()a bit prettier, but as it shows compiler does the right thing without much heroics.