Fix PR56478

On Thu, Mar 07, 2013 at 03:25:35PM +0100, Jakub Jelinek wrote:
> On Tue, Mar 05, 2013 at 10:07:50AM +0100, Marek Polacek wrote:
> > +      if (compare_count.scmp (double_int_zero) == -1)
> > +        compare_count = double_int_zero;
> > +      if (loop_count.scmp (double_int_zero) == -1)
> > +        loop_count = double_int_zero;
> 
> Use if (compare_count.is_negative ()) etc. instead?

Done.

> > -	probability = (double) REG_BR_PROB_BASE * compare_count / loop_count;
> > -      predict_edge (then_edge, PRED_LOOP_IV_COMPARE, probability);
> > +        {
> > +	  tem = compare_count.divmod_with_overflow (loop_count,
> > +						     0, TRUNC_DIV_EXPR,
> > +						     &mod, &of);
> 
> This is wrong.  As compare_count is < loop_count, this will always yield
> zero.
> 
> I guess you want something like:
>   /* If loop_count is too big, such that REG_BR_PROB_BASE * loop_count
>      could overflow, shift both loop_count and compare_count right a bit
>      so that it doesn't overflow.  Note both counts are known not to be
>      negative at this point.  */
>   int clz_bits = clz_hwi (loop_count.high);
>   gcc_assert (REG_BR_PROB_BASE < 32768);
>   if (clz_bits < 16)
>     {
>       loop_count.arshift (16 - clz_bits, HOST_BITS_PER_DOUBLE_INT);
>       compare_count.arshift (16 - clz_bits, HOST_BITS_PER_DOUBLE_INT);
>     }
>   tem = loop_count.mul_with_sign (double_int::from_shwi (REG_BR_PROB_BASE),
> 				  true, &of);
>   gcc_assert (!of);
>   tem = tem.divmod (loop_count, true, TRUNC_DIV_EXPR, &mod);
>   probability = tem.to_uhwi ();
>   predict_edge (then_edge, PRED_LOOP_IV_COMPARE, probability);

Thanks.  How does it look now?  I've discovered that previous patch
caused a regression, so I had to find the bug and retest etc.
(There was loop_count instead of compare_count.)

Regtested/bootstrapped on x86_64-linux, ok for trunk?  Or do you want
me to change anything?

2013-03-08  Marek Polacek  <polacek@redhat.com>
	    Jakub Jelinek <jakub@redhat.com>

	PR tree-optimization/56478
	* predict.c (is_comparison_with_loop_invariant_p): Change the
	type of loop_step to tree.
	(predict_loops): Adjust.
	(predict_iv_comparison): Perform the computations on double_ints.

	* gcc.dg/torture/pr56478.c: New test.


	Marek

On Fri, Mar 08, 2013 at 01:16:37PM +0100, Marek Polacek wrote:
> --- gcc/predict.c.mp	2013-03-07 20:01:01.078417558 +0100
> +++ gcc/predict.c	2013-03-08 11:35:05.227603993 +0100
> @@ -1028,13 +1028,13 @@ static bool
>  is_comparison_with_loop_invariant_p (gimple stmt, struct loop *loop,
>  				     tree *loop_invariant,
>  				     enum tree_code *compare_code,
> -				     int *loop_step,
> +				     tree *loop_step,
>  				     tree *loop_iv_base)
>  {
>    tree op0, op1, bound, base;
>    affine_iv iv0, iv1;
>    enum tree_code code;
> -  int step;
> +  tree step;
>  
>    code = gimple_cond_code (stmt);
>    *loop_invariant = NULL;
> @@ -1077,7 +1077,7 @@ is_comparison_with_loop_invariant_p (gim
>        bound = iv0.base;
>        base = iv1.base;
>        if (host_integerp (iv1.step, 0))
> -	step = tree_low_cst (iv1.step, 0);
> +	step = iv1.step;
>        else
>  	return false;
>      }
> @@ -1086,7 +1086,7 @@ is_comparison_with_loop_invariant_p (gim
>        bound = iv1.base;
>        base = iv0.base;
>        if (host_integerp (iv0.step, 0))
> -	step = tree_low_cst (iv0.step, 0);  
> +	step = iv0.step;
>        else
>  	return false;
>      }

If the callers use double_int computation, perhaps there is
no reason for the host_integerp checks and all we could verify is
that the trees have TREE_CODE (iv0.step) etc. == INTEGER_CST
(and perhaps that if the type of them is unsigned, then they don't
have the msb set (i.e.
!TYPE_UNSIGNED (TREE_TYPE (iv0.step))
|| !tree_to_double_int (iv0.step).is_negative () ).
But perhaps that can be left for 4.9 as an improvement?

> @@ -1224,34 +1224,78 @@ predict_iv_comparison (struct loop *loop
>        && host_integerp (compare_base, 0))

Ditto the host_integerp checks above.

> +      if ((compare_step.scmp (double_int_zero) == 1)

I believe compare_step should be known not to be zero here (there are
integer_zerop checks earlier, aren't they), so this could be also
!compare_step.is_negative ()
?

Other than that it looks good to me, Richard, is this fine for you too?

	Jakub

On Fri, Mar 8, 2013 at 1:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Mar 08, 2013 at 01:16:37PM +0100, Marek Polacek wrote:
>> --- gcc/predict.c.mp  2013-03-07 20:01:01.078417558 +0100
>> +++ gcc/predict.c     2013-03-08 11:35:05.227603993 +0100
>> @@ -1028,13 +1028,13 @@ static bool
>>  is_comparison_with_loop_invariant_p (gimple stmt, struct loop *loop,
>>                                    tree *loop_invariant,
>>                                    enum tree_code *compare_code,
>> -                                  int *loop_step,
>> +                                  tree *loop_step,
>>                                    tree *loop_iv_base)
>>  {
>>    tree op0, op1, bound, base;
>>    affine_iv iv0, iv1;
>>    enum tree_code code;
>> -  int step;
>> +  tree step;
>>
>>    code = gimple_cond_code (stmt);
>>    *loop_invariant = NULL;
>> @@ -1077,7 +1077,7 @@ is_comparison_with_loop_invariant_p (gim
>>        bound = iv0.base;
>>        base = iv1.base;
>>        if (host_integerp (iv1.step, 0))
>> -     step = tree_low_cst (iv1.step, 0);
>> +     step = iv1.step;
>>        else
>>       return false;
>>      }
>> @@ -1086,7 +1086,7 @@ is_comparison_with_loop_invariant_p (gim
>>        bound = iv1.base;
>>        base = iv0.base;
>>        if (host_integerp (iv0.step, 0))
>> -     step = tree_low_cst (iv0.step, 0);
>> +     step = iv0.step;
>>        else
>>       return false;
>>      }
>
> If the callers use double_int computation, perhaps there is
> no reason for the host_integerp checks and all we could verify is
> that the trees have TREE_CODE (iv0.step) etc. == INTEGER_CST
> (and perhaps that if the type of them is unsigned, then they don't
> have the msb set (i.e.
> !TYPE_UNSIGNED (TREE_TYPE (iv0.step))
> || !tree_to_double_int (iv0.step).is_negative () ).
> But perhaps that can be left for 4.9 as an improvement?
>
>> @@ -1224,34 +1224,78 @@ predict_iv_comparison (struct loop *loop
>>        && host_integerp (compare_base, 0))
>
> Ditto the host_integerp checks above.
>
>> +      if ((compare_step.scmp (double_int_zero) == 1)
>
> I believe compare_step should be known not to be zero here (there are
> integer_zerop checks earlier, aren't they), so this could be also
> !compare_step.is_negative ()
> ?
>
> Other than that it looks good to me, Richard, is this fine for you too?

Yes.

Thanks,
Richard.

>         Jakub