[FFmpeg-devel] [PATCH] Optimization of original IFF codec

[Sebastian Vater]

Hey again!

M?ns Rullg?rd a ?crit :
> That one in particular is easier to read before your patch.  If the
> compiler does the right thing with our default settings, leave it.
>   
Although we discussed this yesterday in #ffmpeg-devel I'm replying to
this, so others here who didn't read the log at least know what's going on.

We already found out that the compiler won't replace the / with >> since
signed division differs from bit-shift right. We discussed that I shall
change this stuff to unsigned then.

I'll do a patch for this soon, but I want to mention though, that the
code in IFF does fiddle with bit-planar modes, thus the data is expected
to be calculated bit-wise. In that case I find it more readable to use
<< and >> instead of * and /.

Amiga bit-planar modes differ to chunky in that each pixel is a set bit.
Plane 0 is bit 0, plane 1 is bit 1, etc.
By OR'ing these bits together you get the color index.

Hope this clears things up, that's why I consider it more readable in
that case to also use bit shifting instead mul/div.

To summarize up, for readibility, I would recommend:
Use mul/div for math stuff which should be considered decimal, use <<
and >> for math stuff which should be considered binary (bit-wise).
> That's not how the optimiser works.  If the optimiser runs at all,
> that transformation takes no extra time.
>   
Consider this code in the optimizer:
if ( ASM_INSTRUCTION == X86_DIV && is_log2(dividend) )
  replace_asm ( X86_SHR );
  replace_dividend ( log2(dividend );
}

If the instruction is already shift then the block inside if isn't
executed at all, that what's I meant with the executed code differs.
> The same code runs in both cases.
>   
Yes, but not the same if-conditions yield (see example above), unless
optimizers don't act this way.
>> It also makes absolutely clear that we're dealing with integer
>> arithmetic...a / 2 can either mean divide a float by int, or an int by int.
>>     
>
> No.  / can only mean integer division when operating on integers.
> What to watch out for here is signed vs unsigned division and shift.
> Signed division (of negative values) is not equivalent to a shift.
>   
It looks that you misunderstood me here a bit, imagine the following:
float a;
or:
int a;

[...] some hundred lines of code, then comes a:

a / 2 (yes I know you should write 2.0 then, but you know...)

If you don't remind if a was declared as float or int (sometimes it's
even declared in a different file, the header file), it's unclear here
if it's a float div or int div.
But, if you write:
a >> 1

It's absolutely clear that a MUST be an int...hope this is expresses a
better way I wanted to mention.
> Hopefully you learned something about the optimiser. :-)
>   
Yes, I did indeed. ;-)

-- 

Best regards,
                   :-) Basty/CDGS (-: