Patchwork [tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

login
register
mail settings
Submitter Richard Earnshaw
Date Aug. 17, 2012, 5:05 p.m.
Message ID <502E79BC.3080108@arm.com>
Download mbox | patch
Permalink /patch/178283/
State New
Headers show

Comments

Richard Earnshaw - Aug. 17, 2012, 5:05 p.m.
On 17/08/12 16:20, Richard Earnshaw wrote:
> Ok, in which case we have to give is_widening_mult_rhs_p enough smarts
> to not strip
> 
> 	(s32)u32
> 
> and return u32.
> 
> I'll have another think about it.

Take two.

This version should address your concerns about handling

	(u32)u16 * (u32)u16 -> u64

We now look at each operand directly, but when doing so we check whether
the operand is the same size as the result or not.  When it is, we can
strip any conversion; when it isn't the conversion must preserve
signedness of the inner operand and mustn't be a narrowing conversion.

	* tree-ssa-math-opts.c (widening_mult_conversion_strippable_p):
	New function.
	(is_widening_mult_rhs_p): Use it.

Testing underway (again)

OK?

R.
Andrew Stubbs - Aug. 17, 2012, 7:20 p.m.
On 17/08/12 18:05, Richard Earnshaw wrote:
> Take two.
>
> This version should address your concerns about handling
>
> 	(u32)u16 * (u32)u16 -> u64
>
> We now look at each operand directly, but when doing so we check whether
> the operand is the same size as the result or not.  When it is, we can
> strip any conversion; when it isn't the conversion must preserve
> signedness of the inner operand and mustn't be a narrowing conversion.

So, if I understand correctly, this simply prevents it from stripping 
any conversions from the multiply's right-hand-side if they are not 
widening conversions?

That seems fine to me. Not that I have authority to approve it, of course.

Andrew
Richard Guenther - Aug. 20, 2012, 11:36 a.m.
On Fri, Aug 17, 2012 at 7:05 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
> On 17/08/12 16:20, Richard Earnshaw wrote:
>> Ok, in which case we have to give is_widening_mult_rhs_p enough smarts
>> to not strip
>>
>>       (s32)u32
>>
>> and return u32.
>>
>> I'll have another think about it.
>
> Take two.
>
> This version should address your concerns about handling
>
>         (u32)u16 * (u32)u16 -> u64
>
> We now look at each operand directly, but when doing so we check whether
> the operand is the same size as the result or not.  When it is, we can
> strip any conversion; when it isn't the conversion must preserve
> signedness of the inner operand and mustn't be a narrowing conversion.
>
>         * tree-ssa-math-opts.c (widening_mult_conversion_strippable_p):
>         New function.
>         (is_widening_mult_rhs_p): Use it.
>
> Testing underway (again)
>
> OK?

Ok.

Thanks,
Richard.

> R.
>
Tobias Burnus - Aug. 20, 2012, 2:01 p.m.
Hi Richard,

your patch fails here; I get the build failure:

/projects/tob/gcc-git/gcc/gcc/tree-ssa-math-opts.c: In function ‘bool 
is_widening_mult_rhs_p(tree, tree, tree_node**, tree_node**)’:
/projects/tob/gcc-git/gcc/gcc/tree-ssa-math-opts.c:2014:18: error: 
variable ‘rhs_code’ set but not used [-Werror=unused-but-set-variable]
    enum tree_code rhs_code;
                   ^

Tobias

On 08/17/2012 07:05 PM, Richard Earnshaw wrote:
> --- tree-ssa-math-opts.c	(revision 190502)
> +++ tree-ssa-math-opts.c	(local)

> @@ -1982,9 +2019,7 @@ is_widening_mult_rhs_p (tree type, tree
>         if (is_gimple_assign (stmt))
>   	{
>   	  rhs_code = gimple_assign_rhs_code (stmt);
> -	  if (TREE_CODE (type) == INTEGER_TYPE
> -	      ? !CONVERT_EXPR_CODE_P (rhs_code)
> -	      : rhs_code != FIXED_CONVERT_EXPR)
> +	  if (! widening_mult_conversion_strippable_p (type, stmt))
>   	    rhs1 = rhs;
>   	  else
>   	    {
>

Patch

--- tree-ssa-math-opts.c	(revision 190502)
+++ tree-ssa-math-opts.c	(local)
@@ -1958,6 +1958,43 @@  struct gimple_opt_pass pass_optimize_bsw
  }
 };
 
+/* Return true if stmt is a type conversion operation that can be stripped
+   when used in a widening multiply operation.  */
+static bool
+widening_mult_conversion_strippable_p (tree result_type, gimple stmt)
+{
+  enum tree_code rhs_code = gimple_assign_rhs_code (stmt);
+
+  if (TREE_CODE (result_type) == INTEGER_TYPE)
+    {
+      tree op_type;
+      tree inner_op_type;
+
+      if (!CONVERT_EXPR_CODE_P (rhs_code))
+	return false;
+
+      op_type = TREE_TYPE (gimple_assign_lhs (stmt));
+
+      /* If the type of OP has the same precision as the result, then
+	 we can strip this conversion.  The multiply operation will be
+	 selected to create the correct extension as a by-product.  */
+      if (TYPE_PRECISION (result_type) == TYPE_PRECISION (op_type))
+	return true;
+
+      /* We can also strip a conversion if it preserves the signed-ness of
+	 the operation and doesn't narrow the range.  */
+      inner_op_type = TREE_TYPE (gimple_assign_rhs1 (stmt));
+
+      if (TYPE_UNSIGNED (op_type) == TYPE_UNSIGNED (inner_op_type)
+	  && TYPE_PRECISION (op_type) > TYPE_PRECISION (inner_op_type))
+	return true;
+
+      return false;
+    }
+
+  return rhs_code == FIXED_CONVERT_EXPR;
+}
+
 /* Return true if RHS is a suitable operand for a widening multiplication,
    assuming a target type of TYPE.
    There are two cases:
@@ -1982,9 +2019,7 @@  is_widening_mult_rhs_p (tree type, tree 
       if (is_gimple_assign (stmt))
 	{
 	  rhs_code = gimple_assign_rhs_code (stmt);
-	  if (TREE_CODE (type) == INTEGER_TYPE
-	      ? !CONVERT_EXPR_CODE_P (rhs_code)
-	      : rhs_code != FIXED_CONVERT_EXPR)
+	  if (! widening_mult_conversion_strippable_p (type, stmt))
 	    rhs1 = rhs;
 	  else
 	    {