On Tue, Sep 20, 2016 at 8:47 AM, Gustavo Sverzut Barbieri
<barbi...@gmail.com> wrote:
> On Tue, Sep 20, 2016 at 12:12 PM, Gustavo Sverzut Barbieri
> <barbi...@gmail.com> wrote:
>> On Tue, Sep 20, 2016 at 11:29 AM, Carsten Haitzler <ras...@rasterman.com>
>> wrote:
>>> On Tue, 20 Sep 2016 08:27:25 -0300 Gustavo Sverzut Barbieri
>>> <barbi...@gmail.com> said:
>>>
>>>> > static inline void *
>>>> > @@ -104,12 +101,11 @@ eina_array_foreach(Eina_Array *array, Eina_Each_Cb
>>>> > cb, void *fdata) Eina_Bool ret = EINA_TRUE;
>>>> >
>>>> > EINA_ARRAY_ITER_NEXT(array, i, data, iterator)
>>>> > - if (cb(array, data, fdata) != EINA_TRUE)
>>>> > - {
>>>> > - ret = EINA_FALSE;
>>>> > - break;
>>>> > - }
>>>> > -
>>>> > + {
>>>> > + if (cb(array, data, fdata) == EINA_TRUE) continue;
>>>> > + ret = EINA_FALSE;
>>>> > + break;
>>>> > + }
>>>> > return ret;
>>>> > }
>>>>
>>>>
>>>> for these wouldn't EINA_LIKELY/UNLIKELY be more clear on what is the
>>>> expected/preference to the user AND the compiler. What you did is
>>>> likely to work for one compiler but not the other, if branch
>>>> prediction preference is difference.
>>>
>>> no. likely/unlikely dont do anything useful since 486 architecture days.
>>> back
>>> then the cmp would always predict either true or false (i dont remember) so
>>> the
>>> compiler SHOULD adjust the compare so the more likely outcome is what the
>>> cpu
>>> will then just blindly run while waiting for the result. its is not true
>>> anymore
>>> and these really do nothing useful.
>>
>> it should have the same impact, but admittedly I did not test to guarantee.
>
> Okay, I had to test:
>
> #include <stdio.h>
> int main(int argc, char *argv[]) {
> if (argc < 1)
> puts("less");
> else
> fprintf(stderr, "more: %d", argc);
> return 0;
> }
>
> $ gcc -O2 -S /tmp/a.c -o /tmp/original-order.s
> $ cat /tmp/original-order.s
> .file "a.c"
> .section .rodata.str1.1,"aMS",@progbits,1
> .LC0:
> .string "less"
> .LC1:
> .string "more: %d"
> .section .text.startup,"ax",@progbits
> .p2align 4,,15
> .globl main
> .type main, @function
> main:
> .LFB11:
> .cfi_startproc
> subq $8, %rsp
> .cfi_def_cfa_offset 16
> testl %edi, %edi
> jle .L6
> movl %edi, %edx
> movq stderr(%rip), %rdi
> movl $.LC1, %esi
> xorl %eax, %eax
> call fprintf
> .L3:
> xorl %eax, %eax
> addq $8, %rsp
> .cfi_remember_state
> .cfi_def_cfa_offset 8
> ret
> .L6:
> .cfi_restore_state
> movl $.LC0, %edi
> call puts
> jmp .L3
> .cfi_endproc
> .LFE11:
> .size main, .-main
> .ident "GCC: (GNU) 6.2.1 20160830"
> .section .note.GNU-stack,"",@progbits
>
>
> that is, fprintf() is prefered over puts()... It doesn't read like our
> input code, since -O1 will imply -freorder-blocks.
>
>
> GCC -O2 prefers "else" condition, which can be confirmed by using
> __builtin_expect((argc < 1), 0) -> EINA_UNLIKELY()
>
> $ gcc -O2 -S /tmp/a.c -o /tmp/unlikely.s
> $ diff -u /tmp/original-order.s /tmp/unlikely.s
>
> no changes.
>
>
> now force different prediction by using __builtin_expect((argc < 1),
> 1) -> EINA_LIKELY()
>
> $ gcc -O2 -S /tmp/a.c -o /tmp/likely.s
> $ diff -u /tmp/original-order.s /tmp/likely.s
>
> --- /tmp/original-order.s 2016-09-20 12:31:35.747803857 -0300
> +++ /tmp/likely.s 2016-09-20 12:34:02.599250150 -0300
> @@ -14,22 +14,22 @@
> subq $8, %rsp
> .cfi_def_cfa_offset 16
> testl %edi, %edi
> - jle .L6
> - movl %edi, %edx
> - movq stderr(%rip), %rdi
> - movl $.LC1, %esi
> - xorl %eax, %eax
> - call fprintf
> + jg .L2
> + movl $.LC0, %edi
> + call puts
> .L3:
> xorl %eax, %eax
> addq $8, %rsp
> .cfi_remember_state
> .cfi_def_cfa_offset 8
> ret
> -.L6:
> +.L2:
> .cfi_restore_state
> - movl $.LC0, %edi
> - call puts
> + movl %edi, %edx
> + movq stderr(%rip), %rdi
> + movl $.LC1, %esi
> + xorl %eax, %eax
> + call fprintf
> jmp .L3
> .cfi_endproc
> .LFE11:
>
> that is, puts is now before fprintf. Inverting the branch ourselves,
> no __builtin_expect():
>
> $ gcc -O2 -S /tmp/a.c -o /tmp/
> $ diff -u /tmp/original-order.s /tmp/unlikely.s
>
> --- /tmp/original-order.s 2016-09-20 12:31:35.747803857 -0300
> +++ /tmp/inverted-order.s 2016-09-20 12:35:56.710377570 -0300
> @@ -1,9 +1,9 @@
> .file "a.c"
> .section .rodata.str1.1,"aMS",@progbits,1
> .LC0:
> - .string "less"
> -.LC1:
> .string "more: %d"
> +.LC1:
> + .string "less"
> .section .text.startup,"ax",@progbits
> .p2align 4,,15
> .globl main
> @@ -14,10 +14,10 @@
> subq $8, %rsp
> .cfi_def_cfa_offset 16
> testl %edi, %edi
> - jle .L6
> + jle .L2
> movl %edi, %edx
> movq stderr(%rip), %rdi
> - movl $.LC1, %esi
> + movl $.LC0, %esi
> xorl %eax, %eax
> call fprintf
> .L3:
> @@ -26,9 +26,9 @@
> .cfi_remember_state
> .cfi_def_cfa_offset 8
> ret
> -.L6:
> +.L2:
> .cfi_restore_state
> - movl $.LC0, %edi
> + movl $.LC1, %edi
> call puts
> jmp .L3
> .cfi_endproc
>
> That is, only the stings changed order... the code remained the SAME :-/
>
> Maybe it was pure luck on your side, something we can't trust. IF you
> want to be sure, do use the __builtin_expect().
I have talked with compiler developpers and if we can reproduce this
with __builtin_expect, it would be considered a performance bug. It is
also something that would be interesting to see if it impact ARM and
32 bits system too.
In general I would prefer to rely on EINA_UNLIKELY + goto than the
deeper if chain that this patch pushed in that I find harder to read
and understand.
--
Cedric BAIL
------------------------------------------------------------------------------
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
