[Rawstudio-dev] [PATCH 2 of 7] x86_port: Isolate completly i386 code

[Anders Brander]

Hi,

On Thu, 2006-09-28 at 20:31 +0200, Edouard Gomez wrote:
> > I really don't like this. 'cpuflags' was created especially to avoid
> > polluting the code with #ifdefs.
> > 
> > Are you sure this is the best approach? (I'm not sure it isn't, but I
> > don't like it).
> 
> What's exactly a review is for ! Let's review why i did this, maybe it'l
> turn up  that it wasn't  the best option...  (i already know  the answer
> :-), but i wanted to address that uglyness in a second round of patches)
> 
> 1 - If you have a partial port, may be a x86_64 or a ppc+altivec code,
>     then cpuflags might be initialized so optimized code can be called
>     adequately. BUT then, you can't distinguish which function is
>     optimized or not, and you end calling stub functions in the case a
>     function hasn't been optimized. That was my experience yesterday
>     when i hadn't port the (no)cms rendering pair of functions.

This was exactly what I saw, a partial port - and that is a problem with
stubs, but it's a very limited problem - limited to 32/64-bit i386.

> 2  - So the  core problem  is the  way we  call optimized  functions. As
>      they're just a stub until they get optimized... we should make they
>      complete their contract.  A poor minded solution would  be to put a
>      call to the unoptimized function in the stub so even if
>      a port is incomplete, all functions  do their work. But this is not
>      a solution... if you port to more architecture you'll end with
>      stubs, with optimized functions and lot of if blocks that will end
>      protected by  #define if you  don't separate eg:  _MMX..._SSE space
>      from _ALTIVEC  one, or  a big switch  for each and  every optimized
>      function in the code (though most of them would be empty because no
>      assembler  could  deal  with  their code  for  completly  different
>      archs)...
> 
> The solution to all our problems is called a function pointer !

That is exactly what I used in rs_render_select() as you have noticed.

> The rs_detect_cpu_features i introduced is part of that plan. Instead of
> setting a  simple cpuflags  var, it would  initialize a set  of function
> pointers  according to  what is  available for  each platform  *and* cpu
> type.  Then the  only file  having nasty  #defines would  be rawstudio.c
> (though it's even better to separate that function in its own file), the
> rest of the code just uses function pointers, never dealing with #define
> nor if/switch(cpuflags) blocks.

This sounds like the right thing to do. Feel free to move all this to a
separate file.

> So i advise you  to commit as is for the moment ,  then, i will fix with
> the proper solution in a future patch. Is this plan ok for you ?

This is very much okay with me. Please go ahead.

/abrander