Extend widening_mul pass to handle fixed-point types

Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> writes:
> Hmmm there are regressions after my patch was applied. I had to do some
> rebuilds because trunk appears to be broken for other reasons at the
> minute for ARM which I shall investigate next. 

Yeah, I think it's the other way about: expand_widen_pattern_expr
is using the right type and convert_plusminus_to_widen is using
the wrong one.  The type we pass to optab_for_tree_code determines
the _sign_ of the multiplication, not the size, so we want to pass
in the unwidened type.

> We weren't generating any widening operations before this patch (and I
> suspect my 2 patches are just for fixing the ICE's and doing the right
> thing). If it were just adding support for fixed point arithmetic then
> this is just broken because it shouldn't have changed the way in  which
> code was generated for normal integer operations.  

For the record, the patch wasn't just about adding fixed-point support.
It was also about allowing multiply-accumulate to be used where no plain
multiplication exists.  So the problem for ARM was that the use of the
wrong type in convert_plusminus_to_widen was being hidden by the use of
the right type in convert_mult_widen (i.e. the patch exposed a latent bug).

Could you give the attached patch a try?  It also fixes a case where
the code could create mixed sign-and-unsigned maccs (a bug that existed
before my patch) and a botched call to is_widening_mult_p (a bug
introduced by my patch, sorry).

Richard


gcc/
	* tree-ssa-math-opts.c (convert_plusminus_to_widen): Fix type
	used in the call to optab_for_tree_code.  Fix the second
	is_widening_mult_p call.  Check that both unwidened operands
	have the same sign.

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> writes:
>> Hmmm there are regressions after my patch was applied. I had to do some
>> rebuilds because trunk appears to be broken for other reasons at the
>> minute for ARM which I shall investigate next. 
>
> Yeah, I think it's the other way about: expand_widen_pattern_expr
> is using the right type and convert_plusminus_to_widen is using
> the wrong one.  The type we pass to optab_for_tree_code determines
> the _sign_ of the multiplication, not the size, so we want to pass
> in the unwidened type.
>
>> We weren't generating any widening operations before this patch (and I
>> suspect my 2 patches are just for fixing the ICE's and doing the right
>> thing). If it were just adding support for fixed point arithmetic then
>> this is just broken because it shouldn't have changed the way in  which
>> code was generated for normal integer operations.  
>
> For the record, the patch wasn't just about adding fixed-point support.
> It was also about allowing multiply-accumulate to be used where no plain
> multiplication exists.  So the problem for ARM was that the use of the
> wrong type in convert_plusminus_to_widen was being hidden by the use of
> the right type in convert_mult_widen (i.e. the patch exposed a latent bug).
>
> Could you give the attached patch a try?  It also fixes a case where
> the code could create mixed sign-and-unsigned maccs (a bug that existed
> before my patch) and a botched call to is_widening_mult_p (a bug
> introduced by my patch, sorry).

Now bootstrapped & regression-tested on x86_64-linux-gnu.  Ramana
also confirms that it fixes the ICEs and incorrect arith-rand-ll.c
code on ARM.  OK to install?

Richard

> gcc/
> 	* tree-ssa-math-opts.c (convert_plusminus_to_widen): Fix type
> 	used in the call to optab_for_tree_code.  Fix the second
> 	is_widening_mult_p call.  Check that both unwidened operands
> 	have the same sign.
>
> Index: gcc/tree-ssa-math-opts.c
> ===================================================================
> --- gcc/tree-ssa-math-opts.c	2010-07-28 21:09:59.000000000 +0100
> +++ gcc/tree-ssa-math-opts.c	2010-07-28 21:10:00.000000000 +0100
> @@ -1414,13 +1414,6 @@ convert_plusminus_to_widen (gimple_stmt_
>    else
>      wmult_code = WIDEN_MULT_PLUS_EXPR;
>  
> -  /* Verify that the machine can perform a widening multiply
> -     accumulate in this mode/signedness combination, otherwise
> -     this transformation is likely to pessimize code.  */
> -  this_optab = optab_for_tree_code (wmult_code, type, optab_default);
> -  if (optab_handler (this_optab, TYPE_MODE (type)) == CODE_FOR_nothing)
> -    return false;
> -
>    rhs1 = gimple_assign_rhs1 (stmt);
>    rhs2 = gimple_assign_rhs2 (stmt);
>  
> @@ -1447,37 +1440,49 @@ convert_plusminus_to_widen (gimple_stmt_
>        if (!is_widening_mult_p (rhs1_stmt, &type1, &mult_rhs1,
>  			       &type2, &mult_rhs2))
>  	return false;
> -      mult_rhs1 = fold_convert (type1, mult_rhs1);
> -      mult_rhs2 = fold_convert (type2, mult_rhs2);
>        add_rhs = rhs2;
>      }
>    else if (rhs2_code == MULT_EXPR)
>      {
> -      if (!is_widening_mult_p (rhs1_stmt, &type1, &mult_rhs1,
> +      if (!is_widening_mult_p (rhs2_stmt, &type1, &mult_rhs1,
>  			       &type2, &mult_rhs2))
>  	return false;
> -      mult_rhs1 = fold_convert (type1, mult_rhs1);
> -      mult_rhs2 = fold_convert (type2, mult_rhs2);
>        add_rhs = rhs1;
>      }
>    else if (code == PLUS_EXPR && rhs1_code == WIDEN_MULT_EXPR)
>      {
>        mult_rhs1 = gimple_assign_rhs1 (rhs1_stmt);
>        mult_rhs2 = gimple_assign_rhs2 (rhs1_stmt);
> +      type1 = TREE_TYPE (mult_rhs1);
> +      type2 = TREE_TYPE (mult_rhs2);
>        add_rhs = rhs2;
>      }
>    else if (rhs2_code == WIDEN_MULT_EXPR)
>      {
>        mult_rhs1 = gimple_assign_rhs1 (rhs2_stmt);
>        mult_rhs2 = gimple_assign_rhs2 (rhs2_stmt);
> +      type1 = TREE_TYPE (mult_rhs1);
> +      type2 = TREE_TYPE (mult_rhs2);
>        add_rhs = rhs1;
>      }
>    else
>      return false;
>  
> +  if (TYPE_UNSIGNED (type1) != TYPE_UNSIGNED (type2))
> +    return false;
> +
> +  /* Verify that the machine can perform a widening multiply
> +     accumulate in this mode/signedness combination, otherwise
> +     this transformation is likely to pessimize code.  */
> +  this_optab = optab_for_tree_code (wmult_code, type1, optab_default);
> +  if (optab_handler (this_optab, TYPE_MODE (type)) == CODE_FOR_nothing)
> +    return false;
> +
>    /* ??? May need some type verification here?  */
>  
> -  gimple_assign_set_rhs_with_ops_1 (gsi, wmult_code, mult_rhs1, mult_rhs2,
> +  gimple_assign_set_rhs_with_ops_1 (gsi, wmult_code,
> +				    fold_convert (type1, mult_rhs1),
> +				    fold_convert (type2, mult_rhs2),
>  				    add_rhs);
>    update_stmt (gsi_stmt (*gsi));
>    return true;

On 07/31/2010 09:30 AM, Richard Sandiford wrote:
> Now bootstrapped & regression-tested on x86_64-linux-gnu.  Ramana
> also confirms that it fixes the ICEs and incorrect arith-rand-ll.c
> code on ARM.  OK to install?

I think this is OK.  While looking at it I've been somewhat confused by
the fact that we choose an optab based on the type of the narrow
operands, then choose the right entry in the optab based on the mode of
the wide operand... but that seems unrelated to your patches.

So, please install, and thanks.


Bernd