RFC: fix think-o in tree-ssa-loop-ivopts.c

Still working on PR42505, I found another problem in the ivopts cost 
computation.  This is apparently not the cause of the regression in PR42505 
either, but there's clearly something wrong here.....

This patch

http://gcc.gnu.org/ml/gcc-patches/2009-05/msg01579.html

which was committed as r139821 includes this bit:

>    /* Having offset does not affect runtime cost in case it is added to
>       symbol, but it increases complexity.  */
>    if (offset)
>      cost.complexity++;
>  
> -  cost.cost += n_sums * add_cost (TYPE_MODE (ctype), speed);
> -  return cost;
> +  cost.cost += add_cost (TYPE_MODE (ctype), speed);
> +
> +  aratio = ratio > 0 ? ratio : -ratio;
> +  if (aratio != 1)
> +    cost.cost += multiply_by_cost (aratio, TYPE_MODE (ctype), speed);
>  
>  fallback:
>    {

The "cost" is never referred to again in the fallback code, so it seems like 
deleting the return statement was an unintended mistake.

I quickly put together the attached patch with the obvious fix and re-ran CSiBE 
on x86-64 native....  and I found that code size went up, giving back about half 
the improvement I got from adjusting the iv setup costs for -Os.

So, I'm wondering:  Eric, were the runtime speed improvements you claimed you 
saw here

http://gcc.gnu.org/ml/gcc-patches/2009-05/msg01858.html

actually present in the checked-in version of the patch, where the result of the 
fancy cost computation is simply discarded, or in some other version of the 
patch that implemented what I presume was the intended behavior?  Do we know for 
sure that the fancy cost computation actually even an improvement over the 
fallback case?

TBH, I don't really have time to work on re-checking x86 benchmark results 
myself....  I'm just trying to grok the costs code so I can fix PR42505 for one 
of our ARM customers.  ;-)

-Sandra


2010-06-09  Sandra Loosemore  <sandra@codesourcery.com>

	gcc/
	* tree-ssa-loop-ivopts.c (get_computation_cost_at): Return the
	computed cost.

> The "cost" is never referred to again in the fallback code, so it seems
> like deleting the return statement was an unintended mistake.

Indeed, how embarassing. :-(  This affects 4.5.x as well.

> So, I'm wondering:  Eric, were the runtime speed improvements you claimed
> you saw here
>
> http://gcc.gnu.org/ml/gcc-patches/2009-05/msg01858.html
>
> actually present in the checked-in version of the patch, where the result
> of the fancy cost computation is simply discarded, or in some other version
> of the patch that implemented what I presume was the intended behavior?

Probably in both versions, at least partially.  The main effect (saving 1 
register for loops with non-constant iteration origin) is always present.

> Do we know for sure that the fancy cost computation actually even an
> improvement over the fallback case?

The "fancy" computation was an improvement, yes.

> 2010-06-09  Sandra Loosemore  <sandra@codesourcery.com>
>
> 	gcc/
> 	* tree-ssa-loop-ivopts.c (get_computation_cost_at): Return the
> 	computed cost.

I'd add "again".  Thanks for spotting this.

On Wed, Jun 9, 2010 at 9:59 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> The "cost" is never referred to again in the fallback code, so it seems
>> like deleting the return statement was an unintended mistake.
>
> Indeed, how embarassing. :-(  This affects 4.5.x as well.
>
>> So, I'm wondering:  Eric, were the runtime speed improvements you claimed
>> you saw here
>>
>> http://gcc.gnu.org/ml/gcc-patches/2009-05/msg01858.html
>>
>> actually present in the checked-in version of the patch, where the result
>> of the fancy cost computation is simply discarded, or in some other version
>> of the patch that implemented what I presume was the intended behavior?
>
> Probably in both versions, at least partially.  The main effect (saving 1
> register for loops with non-constant iteration origin) is always present.
>
>> Do we know for sure that the fancy cost computation actually even an
>> improvement over the fallback case?
>
> The "fancy" computation was an improvement, yes.
>
>> 2010-06-09  Sandra Loosemore  <sandra@codesourcery.com>
>>
>>       gcc/
>>       * tree-ssa-loop-ivopts.c (get_computation_cost_at): Return the
>>       computed cost.
>
> I'd add "again".  Thanks for spotting this.

The patch is ok if it passes bootstrap and regtest.  It's also ok for the
branch after a while.

Thanks,
Richard.

> --
> Eric Botcazou
>

> The patch is ok if it passes bootstrap and regtest.  It's also ok for the
> branch after a while.

Thanks.  Sandra, I can take care of this if you wish.

Eric Botcazou wrote:
>> The patch is ok if it passes bootstrap and regtest.  It's also ok for the
>> branch after a while.
> 
> Thanks.  Sandra, I can take care of this if you wish.

That would be great.  I've gotten sucked into another high-priority project and 
wouldn't have time to work on it until next week, probably.

-Sandra

> That would be great.  I've gotten sucked into another high-priority project
> and wouldn't have time to work on it until next week, probably.

OK, bootstrapped/regtested on x86_64-suse-linux, applied on the mainline.