[Mesa-dev] [PATCH] gallium: reflect likely outcome in execution flow

[Ilia Mirkin]

On Thu, May 22, 2014 at 2:56 AM, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> On Wed, 2014-05-21 at 18:05 -0400, Ilia Mirkin wrote:
>
> On Wed, May 21, 2014 at 6:02 PM, Timothy Arceri <t_arceri at yahoo.com.au>
> wrote:
>> Unless we run out of memory the old if statement would always fail so
>> reflect the more likely outcome. Should be be faster most of the time and
>> slightly cleaner looking code.
>>
>> Signed-off-by: Timothy Arceri <t_arceri at yahoo.com.au>
>> ---
>>  src/gallium/auxiliary/util/u_upload_mgr.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/util/u_upload_mgr.c
>> b/src/gallium/auxiliary/util/u_upload_mgr.c
>> index 744ea2e..99f9a08 100644
>> --- a/src/gallium/auxiliary/util/u_upload_mgr.c
>> +++ b/src/gallium/auxiliary/util/u_upload_mgr.c
>> @@ -247,11 +247,10 @@ enum pipe_error u_upload_data( struct u_upload_mgr
>> *upload,
>>     enum pipe_error ret = u_upload_alloc(upload, min_out_offset, size,
>>                                          out_offset, outbuf,
>>                                          (void**)&ptr);
>> -   if (ret != PIPE_OK)
>> -      return ret;
>> +   if (ret == PIPE_OK)
>> +      memcpy(ptr, data, size);
>>
>> -   memcpy(ptr, data, size);
>> -   return PIPE_OK;
>> +   return ret;
>>  }
>
> Have you actually compared the generated code? I find it can be fairly
> instructive to do so... btw, there's a likely() (and unlikely() )
> primitive (in p_compiler.h) which will indicate to gcc whether a
> particular condition is likely or unlikely, so that it can adjust its
> code generation accordingly.
>
>   -ilia
>
>
> Hi Ilia,
>
> I've taken a look at the results (see below) and read up on likely/unlikely
> (I was aware of these but wasn't confident in when they should be used).
>
> The result is my patch avoids a jump which I assume should help with branch
> prediction in the cpu? Something like this is what I was expecting with the
> change.
> The interesting thing is that in this case using likely seems to have added
> an extra 'test' instruction for no added value (please correct me if I'm
> wrong). However there is probably other places in u_upload_alloc(), etc
> where likely might be more helpful.
>
> Tim
>
> Common code:
>
>   444fce: 55                   push   %rbp
>   444fcf: 48 89 e5             mov    %rsp,%rbp
>   444fd2: 48 83 ec 40          sub    $0x40,%rsp
>   444fd6: 48 89 7d e8          mov    %rdi,-0x18(%rbp)
>   444fda: 89 75 e4             mov    %esi,-0x1c(%rbp)
>   444fdd: 89 55 e0             mov    %edx,-0x20(%rbp)
>   444fe0: 48 89 4d d8          mov    %rcx,-0x28(%rbp)
>   444fe4: 4c 89 45 d0          mov    %r8,-0x30(%rbp)
>   444fe8: 4c 89 4d c8          mov    %r9,-0x38(%rbp)
>   444fec: 4c 8d 45 f0          lea    -0x10(%rbp),%r8
>   444ff0: 48 8b 7d c8          mov    -0x38(%rbp),%rdi
>   444ff4: 48 8b 4d d0          mov    -0x30(%rbp),%rcx
>   444ff8: 8b 55 e0             mov    -0x20(%rbp),%edx
>   444ffb: 8b 75 e4             mov    -0x1c(%rbp),%esi
>   444ffe: 48 8b 45 e8          mov    -0x18(%rbp),%rax
>   445002: 4d 89 c1             mov    %r8,%r9
>   445005: 49 89 f8             mov    %rdi,%r8

This is some seriously disgusting code...

444fe8: 4c 89 4d c8          mov    %r9,-0x38(%rbp)
444ff0: 48 8b 7d c8          mov    -0x38(%rbp),%rdi
445005: 49 89 f8             mov    %rdi,%r8

gcc should be able to do better. Do you perhaps have --enable-debug in
your mesa build? That disables -O2... If that is the case, might want
to rerun your analysis without --enable-debug.

  -ilia

>   445008: 48 89 c7             mov    %rax,%rdi
>   44500b: e8 78 fd ff ff       callq  444d88 <u_upload_alloc>
>   445010: 89 45 fc             mov    %eax,-0x4(%rbp)
>   445013: 83 7d fc 00          cmpl   $0x0,-0x4(%rbp)
>
> Pre change:
>
> 0000000000444fce <u_upload_data>:
>   445017: 74 05                je     44501e <u_upload_data+0x50>
>   445019: 8b 45 fc             mov    -0x4(%rbp),%eax
>   44501c: eb 1b                jmp    445039 <u_upload_data+0x6b>
>   44501e: 8b 55 e0             mov    -0x20(%rbp),%edx
>   445021: 48 8b 45 f0          mov    -0x10(%rbp),%rax
>   445025: 48 8b 4d d8          mov    -0x28(%rbp),%rcx
>   445029: 48 89 ce             mov    %rcx,%rsi
>   44502c: 48 89 c7             mov    %rax,%rdi
>   44502f: e8 2c 40 c0 ff       callq  49060 <memcpy at plt>
>   445034: b8 00 00 00 00       mov    $0x0,%eax
>   445039: c9                   leaveq
>   44503a: c3                   retq
>
>
> Post change:
>
> 0000000000444fce <u_upload_data>:
>   445017: 75 16                jne    44502f <u_upload_data+0x61>
>   445019: 8b 55 e0             mov    -0x20(%rbp),%edx
>   44501c: 48 8b 45 f0          mov    -0x10(%rbp),%rax
>   445020: 48 8b 4d d8          mov    -0x28(%rbp),%rcx
>   445024: 48 89 ce             mov    %rcx,%rsi
>   445027: 48 89 c7             mov    %rax,%rdi
>   44502a: e8 31 40 c0 ff       callq  49060 <memcpy at plt>
>   44502f: 8b 45 fc             mov    -0x4(%rbp),%eax
>   445032: c9                   leaveq
>   445033: c3                   retq
>
>
> With likely:
>
> 0000000000444fce <u_upload_data>:
>   44501d: 48 85 c0             test   %rax,%rax
>   445020: 74 16                je     445038 <u_upload_data+0x6a>
>   445022: 8b 55 e0             mov    -0x20(%rbp),%edx
>   445025: 48 8b 45 f0          mov    -0x10(%rbp),%rax
>   445029: 48 8b 4d d8          mov    -0x28(%rbp),%rcx
>   44502d: 48 89 ce             mov    %rcx,%rsi
>   445030: 48 89 c7             mov    %rax,%rdi
>   445033: e8 28 40 c0 ff       callq  49060 <memcpy at plt>
>   445038: 8b 45 fc             mov    -0x4(%rbp),%eax
>   44503b: c9                   leaveq
>   44503c: c3                   retq
>