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

[Måns Rullgård]

Sebastian Vater <cdgs.basty at googlemail.com> writes:

> 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.

I disagree.  Multiplication and division is best written as such when
the compiler is smart enough to generate shifts in the actual output.

> 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).

For bit-masks I agree shifts should be used.  The cases you changed
were _numbers_, not bit-masks.

>> 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.

There is no such code in the optimiser.  In of the last stages of
compilation (in gcc), the RTL representation of the code is matched
against various patterns defined in the machine description files.
Each matching pattern then translates into real instructions for a
code fragment.  A pattern matching integer division will output shift
instructions, just like the pattern for a shift will.  The executed
code (in the compiler) is mostly the same.

Besides, if trivial things like this are buggy, you should really try
to find a better compiler.

>> 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:

If your declarations are separated from code by hundreds of lines,
you're doing something wrong.  In good code, it is also clear from the
context which variables are integers and which are floats.

-- 
M?ns Rullg?rd
mans at mansr.com