<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN"> <HTML> <HEAD> <META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8"> <META NAME="GENERATOR" CONTENT="GtkHTML/4.6.6"> </HEAD> <BODY> On Wed, 2014-05-21 at 18:05 -0400, Ilia Mirkin wrote: <BLOCKQUOTE TYPE=CITE> <PRE> On Wed, May 21, 2014 at 6:02 PM, Timothy Arceri <<A HREF="mailto:t_arceri@yahoo.com.au">t_arceri@yahoo.com.au</A>> 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 <<A HREF="mailto:t_arceri@yahoo.com.au">t_arceri@yahoo.com.au</A>> > --- > 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 </PRE> </BLOCKQUOTE> 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 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@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@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@plt> 445038: 8b 45 fc mov -0x4(%rbp),%eax 44503b: c9 leaveq 44503c: c3 retq </BODY> </HTML>