[hatari-devel] Hatari patches for bitplane conversion

Kåre Andersen kareandersen at gmail.com
Sat Jul 25 20:19:46 CEST 2009


Top posting to say: Ok! Will improve it according to your guidelines
and send a new version. Oh,and the use of the inline keyword is meant
as an explanation in the source rather than an idea of forcing the
compiler to do anything. And the "globals" (file scope) were made so
for the sake of avoiding all that repetition. I guess you are right
about it being ugly, so away they go :)

Stay tuned!

/Kåre

On Sat, Jul 25, 2009 at 8:08 PM, Eero Tamminen<eerot at users.berlios.de> wrote:
> Hi,
>
> On Saturday 25 July 2009, Kåre Andersen wrote:
>> Here we go :)
>>
>> I put the 32-bit converters into one file + header- shifter.c /
>> shifter.h - made some inline functions (kicks in at -O3 atleast) in an
>> attempt to refactor a bit and make it maintainable, and I did some
>> testing.
>>
>> So far so good! Not able to find bad cases yet. Please review.
>
> Hm.  The new code needs quite a bit of work to be acceptable.
>
> * The names of the files as Nicolas commented...
> * There are too many variables that are global within the object scope.
>   For p0,p1,p2,p3 it might sense to have them as object global, for others
>   I don't see any reason.
> * It doesn't make sense to have stuff that can be done once in screen.c
>  added to all conversion functions:
>  - the debug "fprintf(stderr,"blitFrom: %d blitTo: %d", blitFrom, blitTo);"
>  - initConvert().  If there's something that cannot go to screen.c caller,
>    it should be left to convertion function
> * Headers should have function and variable _declarations_ + defines +
>  macros & static inlines (i.e. API) used by multiple *.c files, not actual
>  variables and function bodies like shifter.h has.  I don't see anything
>  in it currently that would actually need to be there instead of directly
>  in the *.c file?
> * As to inline... There's no such thing as a (global) inline, that's
>  a logical impossibility.  Only valid construct is "static inline".
>  A C99 compiler should've given you a warning or error about that?
>
>
> So... For example this:
> --------------------
> inline void getColMed(Uint32 m)
> {
>        col  = (p0 &m) ? 0x1 : 0;
>        col |= (p1 &m) ? 0x2 : 0;
>        col = STRGBPalette[col];
> }
> --------------------
>
> Should be in the *.c file and look like this:
> --------------------
> static inline Uint32 getColMed(Uint32 m)
> {
>        Uint32 col;
>        col  = (p0 &m) ? 0x1 : 0;
>        col |= (p1 &m) ? 0x2 : 0;
>        return STRGBPalette[col];
> }
> --------------------
>
> Besides the resulting code being more readable that way, in some cases it's
> also more efficient as compiler can optimize (code calling) functions
> without side-effects better[1].
>
> Here you're dealing with object-global static variables so compiler probably
> still optimize them as well, but I think it's better to always code cleanly
> & in a way that will perform better in all circumstances.
>
> Btw. With GCC, "inline" is pretty redundant, GCC does its own decisions
> about whether something's going to be inlined regardless of the inline
> keyword. It's a bit like the obsolete "register" keyword for variables.
> If you just use "static" to indicate correct logical/lexical scope, GCC will
> automatically inline functions where it's appropriate (e.g. they're called
> once or very small) or just jump to them without setting up a function call
> & cleanup.
>
> [1] For non-static functions one can tell GCC that the function doesn't
> have side-effects with attributes like __pure and __const.  See:
>        http://blog.rlove.org/2005/10/with-little-help-from-your-compiler.html
>
>
> This part could also be simpliefied & optimized further:
> -------------
>                update = AdjustLinePaletteRemap(y) & PALETTEMASK_UPDATEMASK;
>                lineChanged =
>                        update | pFrameBuffer->bFullUpdate |
>                                wmemcmp(stplanes, stcopy, no_words);
>
>                bScreenContentsChanged |= lineChanged;
>                trackBounds();
>
>                for( x = 0; x < STScreenWidthBytes>>3 && lineChanged; x++)
> -------------
>
> * It's redundant to have both "update" and "lineChanged", use only one.
>
> * With "blitFrom" and "blitTo" variables, "bScreenContentsChanged"
>  is redundant. You can check screen changes (in screen.c) with:
>        if (blitFrom < blitTo)
>                bScreenContentsChanged = true;
>  or remove "bScreenContentsChanged" from screen.c too.
>
> * Do like in other functions:
> -               for( x = 0; x < STScreenWidthBytes>>3 && lineChanged; x++)
> +               for( x = 0; lineChanged && x < STScreenWidthBytes>>3; x++)
>
> Please also verify from the generated code that "lineChanged" isn't checked
> on every round of this loop.  If it is, use instead:
> -----------------
>                if (lineChanged) {
>                        continue;
>                        /* Offset to next line: */
>                        pPCScreenDest = (((Uint8 *)pPCScreenDest)+PCScreenBytesPerLine);
>                }
>                for( x = 0; x < STScreenWidthBytes>>3; x++)
> -----------------
>
> Btw. It's quite ugly that covertion functions change screen.c pPCScreenDest
> variable (like it's done also in current Hatari code).  It would be cleaner
> and faster if this value would be given as argument to the conversion
> functions so that the value goes to a local variable.
>
>
>        - Eero
>



More information about the hatari-devel mailing list