Re: [gst-cvs] gstreamer: micro-optim: if (x) is cheaper than if (x > 0) for unsigned integers

[Tim-Philipp Müller-2]

On Fri, 2010-12-03 at 03:04 -0800, Edward Hervey wrote:

> Module: gstreamer
> Branch: master
> Commit: 6aa8ca37eeb9debfa6919741a023250bf278248f
> URL:    http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=6aa8ca37eeb9debfa6919741a023250bf278248f
>
> Author: Edward Hervey <[hidden email]>
> Date:   Sat Apr 11 15:04:41 2009 +0200
>
> micro-optim: if (x) is cheaper than if (x > 0) for unsigned integers

I really really dislike this change. Is it really worth it?

I think this compromises code readability too much.

Cheers
 -Tim



------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Edward Hervey]

On Fri, 2010-12-03 at 11:47 +0000, Tim-Philipp Müller wrote:

> On Fri, 2010-12-03 at 03:04 -0800, Edward Hervey wrote:
>
> > Module: gstreamer
> > Branch: master
> > Commit: 6aa8ca37eeb9debfa6919741a023250bf278248f
> > URL:    http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=6aa8ca37eeb9debfa6919741a023250bf278248f
> >
> > Author: Edward Hervey <[hidden email]>
> > Date:   Sat Apr 11 15:04:41 2009 +0200
> >
> > micro-optim: if (x) is cheaper than if (x > 0) for unsigned integers
>
> I really really dislike this change. Is it really worth it?

  I saw gcc asm where it was doing an expensive compare as opposed to a
fast != 0. So yes, needed. Worth it ? Well I did title it "micro-optim".

>
> I think this compromises code readability too much.

  You mean if(x) is ... unreadable ? Isn't that C 101 ?

    Edward

>
> Cheers
>  -Tim
>
>



------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Wes Miller]

Take it from a visually impaired person, if (x) and if (x>0) are both quite readable compared to the slammed together, no whitespace, { on the end-of-line eye-tests some people churn out.  Thank God for beautifiers.

And besides, good practice says that ought to be if (0<x) as a form of the practice to prevent the dreader (if(x=0) // single =.

Wes

[Xavier Bestel]

On Fri, 2010-12-03 at 12:59 +0100, Edward Hervey wrote:
> > > micro-optim: if (x) is cheaper than if (x > 0) for unsigned integers
> >
> > I really really dislike this change. Is it really worth it?
>
>   I saw gcc asm where it was doing an expensive compare as opposed to a
> fast != 0. So yes, needed. Worth it ? Well I did title it "micro-optim".

I'd say the bug is with gcc, not gstreamer.
A different compiler may generate correct code for this.

        Xav


------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[David Schleef-2]

On Fri, Dec 03, 2010 at 12:59:33PM +0100, Edward Hervey wrote:

> On Fri, 2010-12-03 at 11:47 +0000, Tim-Philipp Müller wrote:
> > On Fri, 2010-12-03 at 03:04 -0800, Edward Hervey wrote:
> >
> > > Module: gstreamer
> > > Branch: master
> > > Commit: 6aa8ca37eeb9debfa6919741a023250bf278248f
> > > URL:    http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=6aa8ca37eeb9debfa6919741a023250bf278248f
> > >
> > > Author: Edward Hervey <[hidden email]>
> > > Date:   Sat Apr 11 15:04:41 2009 +0200
> > >
> > > micro-optim: if (x) is cheaper than if (x > 0) for unsigned integers
> >
> > I really really dislike this change. Is it really worth it?
>
>   I saw gcc asm where it was doing an expensive compare as opposed to a
> fast != 0. So yes, needed. Worth it ? Well I did title it "micro-optim".

What is an expensive compare?  cmp $0, %eax and test %eax, %eax are
both one cycle.  It is somewhat amusing that gcc-4.5 generates
different code for these, when it should know they are identical.
gcc-4.4 doesn't, so I consider that a regression, but certainly not
an important one.

> Date:   Sat Apr 11 15:04:41 2009 +0200

Hiding patches in our local tree, eh?



David


------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Tim-Philipp Müller-2]

On Fri, 2010-12-03 at 14:17 -0500, David Schleef wrote:
 
> >   I saw gcc asm where it was doing an expensive compare as opposed to a
> > fast != 0. So yes, needed. Worth it ? Well I did title it "micro-optim".
>
> What is an expensive compare?  cmp $0, %eax and test %eax, %eax are
> both one cycle.  It is somewhat amusing that gcc-4.5 generates
> different code for these, when it should know they are identical.
> gcc-4.4 doesn't, so I consider that a regression, but certainly not
> an important one.

Edward, in light of David's comments (and your own), can we revert this
please?

(At the very least I'd like to see the not-quite-right parts like the
refcount checks where refcount is a signed int reverted and where it
used to warn on refcount <= 0, but now only warns on refcount == 0)

Cheers
 -Tim



------------------------------------------------------------------------------
Lotusphere 2011
Register now for Lotusphere 2011 and learn how
to connect the dots, take your collaborative environment
to the next level, and enter the era of Social Business.
http://p.sf.net/sfu/lotusphere-d2d
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel