[hatari-devel] Hatari patches for bitplane conversion

Eero Tamminen eerot at users.berlios.de
Sat Jul 25 20:08:33 CEST 2009


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