[FFmpeg-devel] [PATCH 2/9] avcodec/gdv: Replace divisions by shifts in rescale()

[James Almer]

On 8/5/2018 6:58 PM, Michael Niedermayer wrote:
> On Sun, Aug 05, 2018 at 06:19:01PM -0300, James Almer wrote:
>> On 8/5/2018 5:29 PM, Michael Niedermayer wrote:
>>> Divisions tend to be slower than shifts unless the compiler optimizes them out.
>>> And some of these are in inner loops.
>>
>> This patch makes the code slightly less readable, IMO. What compiler
>> doesn't optimize out an integer division by 2 into a right shift?
>> Is this part of the cause for the timeouts you mention in patch 9/9? Do
>> those run on builds with compiler optimizations disabled then?
> 
> Iam running them with -O1
> i dont know if gcc succeeded optimizing the /2 to a plain shift. (it has
> to proof that the value is positive as nicolas said)
> Its more a habit of not doing potentially slow operations in inner loops.
> 
> As in "always favor shift over division when possibly" vs. "spend time
> proofing that the speedloss either doesnt matter or that every supported
> compiler can optimize it"
> 
> 
> 
>>
>> The other patches in this set look good since they effectively simplify
>> and optimize the code for all scenarios (memcpy vs loop, reduced amount
>> of iterations, etc). But this one is trying to work around a decision
>> made by a compiler in a non real world scenario with specific runtime
>> constrains.
>> Does this timeout still happen if you apply the entire set except for
>> this patch?
> 
> Ill have to try but i dont think its wise to leave divisions in inner
> loops where they are easy avoidable.
> also makes greping for such suboptimal code harder, if anyone tried that
> as in "divisions in a loop"
> 
> but ye, if people prefer to leave that, we can do that (assuming it
> isnt slower), should i test this ?

No, don't bother. If a compiler needs to proof the value is positive
before optimizing the division into a shift then it's a different situation.
Maybe make these unsigned as suggested by Nicolas instead of changing
division into shift, which will also address the readability concerns,
but otherwise this patch is ok.