Mailinglist Archive: radeonhd (330 mails)

["Conn Clark" <conn.o.clark@xxxxxxxxx>]

From: Conn Clark <conn.o.clark@xxxxxxxxx>
Date: Fri, Oct 24, 2008 at 1:11 PM
Subject: Re: [radeonhd] faster RHDRegRead, RHDRegWrite, and RHDRegMask patch
To: Darin McBride <dmcbride@xxxxxxxxxxxx>


On Thu, Oct 23, 2008 at 6:57 PM, Darin McBride <dmcbride@xxxxxxxxxxxx> wrote:
> On Thursday 23 October 2008 4:48:40 pm Conn Clark wrote:
> > Hello,
> >
> > Here is a patch to speed up RHDRegRead, RHDRegWrite, and RHDRegMask .
> > I have just implemented these functions as macros since gcc doesn't
> > always inline these functions.
>
> Minor(?) points/questions:
>
> a) did you profile the inline versions vs macros vs what is in the source tree
> right now?  I'm not sure that your macros will provide huge improvements over
> letting gcc figure out when to inline and when not to.  Especially considering
> how much less effort it is to get inline functions working properly as
> compared to macros.  (I'll take "working" over "fast" any day.)

  Literally profile no,  benchmark yes. Using glxgears I was able to
get 200 fps with the base code. With my patch I was able to get 211
fps on my laptop's non accelerated radeon 3100.  This falls out side
of the typical margin of error of about 5 fps that I have observed.
If you were to compile to assembly you would see just how little
inlining actually occurs. Luc Verhaegen resorted to macros in his CS
code because he couldn't deny the benchmark results.

> b) when creating these macros, you really need to be careful about all usage
> scenarios.  You're using each parameter only once in most of your macros,
> that's good.  But there are some cases that will look like valid C that will
> produce funky results.  This is why macros are often inside "do {...}
> while(0)" structures.
>
> if (RHDRegWritex(a, b, c) & SOME_MASK)
> {
> }
>
> This is going to do really weird things.  But if you put your macro into a do-
> while-zero loop, it'll simply fail to compile (which is much better than doing
> weird things at runtime).
>
> if (*(volatile CARD32 *)((CARD8 *) RHDPTR(xf86Screens[a])->MMIOBase + b) = c &
> SOME_MASK)
>
> You can see where "c & SOME_MASK" will be evaluated before the assignment.
> With a macro of:
>
> #define _RHDRegWritex(scrnIndex, offset, value)  \
>  do { \
>    *(volatile CARD32 *)((CARD8 *) RHDPTR(xf86Screens[scrnIndex])->MMIOBase \
>         + offset) = value \
>  while (0)
>
> the above if statement will definitely preprocess into something that doesn't
> compile.  Then the developer will know not to do something silly like this.
> ;-)
>
> Note that your RHDRegMaskx macro uses all parameters multiple times.  This
> will break down if it's used in a loop with, say, value being "x <<= 2" or
> calling a long-running function (with "long" being relative here), or a
> function that is an iterator (i.e., returns the next value each time), or
> whatever.  Similarly if it's the offset changing each time or whatever.  An
> inline function doesn't suffer from these problems.

 This is only about the 3rd macro I have ever written. So its obvious
I lack experience with writing them. I don't expect this patch to be
included as is. I will try and correct things as you suggest and
resubmit it. My macros do work in the code however. I wrote it so that
people could try it and see for themselves if using macros is the way
to go.

 Personally I don't like using macros or inline functions for such
simple operations. I always code things directly because it allows not
only me to see potential improvements but also allows the compiler to
use its optimizers to the fullest extent.. Since this is not my code
and others need to be able to read, understand, and modify it, it was
best for me to try and implement this as a macro so that I could share
it with others.


Please try the code and tell me what you think. Its easily removed if
you do find a problem. I think I had less tearing moving windows
around with it.

Thank you for your feed back on my coding. I learned some things.

Conn

--

Conn O. Clark

Observation: In formal computer science advances are made
by standing on the shoulders of giants. Linux has proved
that if there are enough of you, you can advance just as
far by stepping on each others toes.
-- 
To unsubscribe, e-mail: radeonhd+unsubscribe@xxxxxxxxxxxx
For additional commands, e-mail: radeonhd+help@xxxxxxxxxxxx