Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Richard Earnshaw]

On 20/08/12 12:36, Richard Guenther wrote:
 On Fri, Aug 17, 2012 at 7:05 PM, Richard Earnshaw rearn...@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.

 


It turns out that this patch caused a few tests in gcc.target/arm to
start failing.  Investigating has shown that there are a number of
reasons for this.

One test (gcc.target/arm/pr50318-1.c) is just bogus.  It's scanning for
an unsigned widening multiply when the correct operation is a signed
widening multiply.  Fixing the compiler has just exposed the broken test.

Two tests (gcc.target/arm/smlaltb-1.c and gcc.target/arm/smlaltt-1.c)
fail because the compiler now generates a subreg in the middle of the
widening multiply expression.  Our patterns don't recognize this form
and I'm really not keen on the compiler doing this sort of thing anyway,
subreg operations of this nature are endian dependent and dealing with
this is messy.  For now I'm going to xfail these two tests.

The final two failures (gcc.target/arm/wmul-5.c and
gcc.target/arm/wmul-6.c) really should pass.  They don't because my
first iteration of the patch failed to spot that (sign_extend:DI
(zero_extend:SI (reg:HI))) can be simplified legitimately to
(zero_extend:DI (reg:HI)).  The patch below to
widening_mult_conversion_strippable_p fixes this.

So this is a clean-up patch to fix these issues.

* tree-ssa-math-opts.c (widening_mult_conversion_strippable_p):
Sign-extension of a zero-extended value can be simplified to
just zero-extension.

testsuite:

* gcc.target/arm/pr50318-1.c: Scan for smlal.
* gcc.target/arm/smlaltb-1.c: XFAIL test.
* gcc.target/arm/smlaltt-1.c: Likewise.

OK?


--- testsuite/gcc.target/arm/pr50318-1.c(revision 190999)
+++ testsuite/gcc.target/arm/pr50318-1.c(local)
@@ -8,4 +8,4 @@ long long test (unsigned int sec, unsign
long)nsecs;
 }
 
-/* { dg-final { scan-assembler umlal } } */
+/* { dg-final { scan-assembler smlal } } */
--- testsuite/gcc.target/arm/smlaltb-1.c(revision 190999)
+++ testsuite/gcc.target/arm/smlaltb-1.c(local)
@@ -11,4 +11,4 @@ foo (long long x, int in)
   return x + b * a;
 }
 
-/* { dg-final { scan-assembler smlaltb\\t } } */
+/* { dg-final { scan-assembler smlaltb\\t { xfail *-*-* } } } */
--- testsuite/gcc.target/arm/smlaltt-1.c(revision 190999)
+++ testsuite/gcc.target/arm/smlaltt-1.c(local)
@@ -11,4 +11,4 @@ foo (long long x, int in1, int in2)
   return x + b * a;
 }
 
-/* { dg-final { scan-assembler smlaltt\\t } } */
+/* { dg-final { scan-assembler smlaltt\\t { xfail *-*-* } } } */
--- tree-ssa-math-opts.c(revision 190999)
+++ tree-ssa-math-opts.c(local)
@@ -1985,7 +1985,11 @@ widening_mult_conversion_strippable_p (t
 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)
+  /* If the inner-most type is unsigned, then we can strip any
+intermediate widening operation.  If it's signed, then the
+intermediate widening operation must also be signed.  */
+  if ((TYPE_UNSIGNED (inner_op_type)
+  || TYPE_UNSIGNED (op_type) == TYPE_UNSIGNED (inner_op_type))
   TYPE_PRECISION (op_type)  TYPE_PRECISION (inner_op_type))
return true;
 

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Richard Guenther]

On Fri, Sep 7, 2012 at 10:35 AM, Richard Earnshaw rearn...@arm.com wrote:
 On 20/08/12 12:36, Richard Guenther wrote:
 On Fri, Aug 17, 2012 at 7:05 PM, Richard Earnshaw rearn...@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.




 It turns out that this patch caused a few tests in gcc.target/arm to
 start failing.  Investigating has shown that there are a number of
 reasons for this.

 One test (gcc.target/arm/pr50318-1.c) is just bogus.  It's scanning for
 an unsigned widening multiply when the correct operation is a signed
 widening multiply.  Fixing the compiler has just exposed the broken test.

 Two tests (gcc.target/arm/smlaltb-1.c and gcc.target/arm/smlaltt-1.c)
 fail because the compiler now generates a subreg in the middle of the
 widening multiply expression.  Our patterns don't recognize this form
 and I'm really not keen on the compiler doing this sort of thing anyway,
 subreg operations of this nature are endian dependent and dealing with
 this is messy.  For now I'm going to xfail these two tests.

 The final two failures (gcc.target/arm/wmul-5.c and
 gcc.target/arm/wmul-6.c) really should pass.  They don't because my
 first iteration of the patch failed to spot that (sign_extend:DI
 (zero_extend:SI (reg:HI))) can be simplified legitimately to
 (zero_extend:DI (reg:HI)).  The patch below to
 widening_mult_conversion_strippable_p fixes this.

 So this is a clean-up patch to fix these issues.

 * tree-ssa-math-opts.c (widening_mult_conversion_strippable_p):
 Sign-extension of a zero-extended value can be simplified to
 just zero-extension.

 testsuite:

 * gcc.target/arm/pr50318-1.c: Scan for smlal.
 * gcc.target/arm/smlaltb-1.c: XFAIL test.
 * gcc.target/arm/smlaltt-1.c: Likewise.

 OK?

Ok.

Thanks,
Richard.

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Richard Guenther]

On Fri, Aug 17, 2012 at 7:05 PM, Richard Earnshaw rearn...@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.

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Tobias Burnus]

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
{

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Richard Earnshaw]

On 20/08/12 15:01, Tobias Burnus wrote:
 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
  {

 
 


Whoops!  Sorry about that.

Fixed thusly.  Committed as obvious.

PR tree-ssa/54295
* tree-ssa-math-opts.c (is_widening_mult_rhs_p): Delete rhs_code
declaration and setter.

R.

Index: tree-ssa-math-opts.c
===
--- tree-ssa-math-opts.c(revision 190533)
+++ tree-ssa-math-opts.c(working copy)
@@ -2011,14 +2011,12 @@ is_widening_mult_rhs_p (tree type, tree 
 {
   gimple stmt;
   tree type1, rhs1;
-  enum tree_code rhs_code;
 
   if (TREE_CODE (rhs) == SSA_NAME)
 {
   stmt = SSA_NAME_DEF_STMT (rhs);
   if (is_gimple_assign (stmt))
{
- rhs_code = gimple_assign_rhs_code (stmt);
  if (! widening_mult_conversion_strippable_p (type, stmt))
rhs1 = rhs;
  else

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

[Richard Earnshaw]

PR54295 shows that widening multiply-accumulate operations can end up
with incorrect code.  The path to the failure is as follows

1) Compiler detects a widening multiply operation and generates the
correct widening-multiply operation.  This involves stripping off the
widening cast to leave the underlying operands.

That is

X = (c1) a * (c2) b
=
X = a w* b

the type of w is picked based on the types of a and b; if both are
signed a signed multiply-extend operation is used; if both are unsigned
an unsigned multiply-extend is used.  If they differ a signed/unsigned
multiply is used, if it exists.  If either a or b is a positive constant
it is coerced, if possible, into the type of the other operand.

2) The compiler then notices that the result, X is used in an addition
operation

Y = X + d

As X is a widening multiply, the compiler then looks inside it to try
and generate a widening-multiply-accumulate operation.  However, it
passes the rewritten expression (X = a w* b) to is_widening_mult_p and
that routine does not correctly check what type of multiply is being
done: the code blindly tries to strip of an conversion operations.

The failure happens when a is also the result of a cast operation, for
example, being cast from an unsigned to an int.  The compiler then
re-formulates a signed multiply-extend into an unsigned multiply-extend.
 That is, if
a = (int) e // typeof(e) == unsigned

Then instead of
Y = WIDEN_MULT_PLUS (a, b, d)
we end up with
Y = WIDEN_MULT_PLUS (e, b, d)

The fix is to make is_widening_mult_p note that it has been called with
a WIDEN_MULT_EXPR and rather than decompose the operands again, to
simply extract the existing operands, which have already been formulated
correctly for a widening multiply operation.

PR tree-ssa/54295
* tree-ssa-math-opts.c (is_widening_mult_p): Short-circuit when
the stmt is already a widening multiply.

Testsuite

PR tree-ssa/54295
* gcc.c-torture/execute/20120817-1.c: New test.

OK to commit once testing has completed?

--- tree-ssa-math-opts.c(revision 190462)
+++ tree-ssa-math-opts.c(local)
@@ -2039,6 +2039,25 @@ is_widening_mult_p (gimple stmt,
TREE_CODE (type) != FIXED_POINT_TYPE)
 return false;
 
+  /* If we already have a widening multiply expression, simply extract the
+ operands.  */
+  if (gimple_assign_rhs_code (stmt) == WIDEN_MULT_EXPR)
+{
+  *rhs1_out = gimple_assign_rhs1 (stmt);
+  *rhs2_out = gimple_assign_rhs2 (stmt);
+  if (TREE_CODE (*rhs1_out) == INTEGER_CST)
+   *type1_out = TREE_TYPE (*rhs2_out);
+  else
+   *type1_out = TREE_TYPE (*rhs1_out);
+
+  if (TREE_CODE (*rhs2_out) == INTEGER_CST)
+   *type2_out = TREE_TYPE (*rhs1_out);
+  else
+   *type2_out = TREE_TYPE (*rhs2_out);
+
+  return true;
+}
+
   if (!is_widening_mult_rhs_p (type, gimple_assign_rhs1 (stmt), type1_out,
   rhs1_out))
 return false;
Index: 20120817-1.c
===
--- 20120817-1.c(revision 0)
+++ 20120817-1.c(revision 0)
@@ -0,0 +1,14 @@
+typedef unsigned long long u64;
+unsigned long foo = 0;
+u64 f() __attribute__((noinline));
+
+u64 f() {
+  return ((u64)40) + ((u64) 24) * (int)(foo - 1);
+}
+
+int main ()
+{
+  if (f () != 16)
+abort ();
+  exit (0);
+}

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Andrew Stubbs]

On 17/08/12 15:04, Richard Earnshaw wrote:

The fix is to make is_widening_mult_p note that it has been called with
a WIDEN_MULT_EXPR and rather than decompose the operands again, to
simply extract the existing operands, which have already been formulated
correctly for a widening multiply operation.


As long as the existing test cases work, I think the only problem with 
this idea is if some architecture has a wider range of widening 
multiply-and-accumulate than it does plain widening multiply.


If no such architecture exists then this is fine with me, for whatever 
that's worth. IIRC, ARM is one of only two architectures (in GCC) with 
widening multiplies that widen more than twice the width, and, last I 
looked, the only one that has the patterns to use this code, so it's 
probably safe.


Andrew

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Richard Earnshaw]

On 17/08/12 15:22, Andrew Stubbs wrote:
 On 17/08/12 15:04, Richard Earnshaw wrote:
 The fix is to make is_widening_mult_p note that it has been called with
 a WIDEN_MULT_EXPR and rather than decompose the operands again, to
 simply extract the existing operands, which have already been formulated
 correctly for a widening multiply operation.
 
 As long as the existing test cases work, I think the only problem with 
 this idea is if some architecture has a wider range of widening 
 multiply-and-accumulate than it does plain widening multiply.

But surely in that case step1 wouldn't have applied and we'd then be
looking at a MULT_EXPR not a WIDEN_MULT_EXPR.

R.

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Andrew Stubbs]

On 17/08/12 15:31, Richard Earnshaw wrote:

On 17/08/12 15:22, Andrew Stubbs wrote:

On 17/08/12 15:04, Richard Earnshaw wrote:

The fix is to make is_widening_mult_p note that it has been called with
a WIDEN_MULT_EXPR and rather than decompose the operands again, to
simply extract the existing operands, which have already been formulated
correctly for a widening multiply operation.


As long as the existing test cases work, I think the only problem with
this idea is if some architecture has a wider range of widening
multiply-and-accumulate than it does plain widening multiply.


But surely in that case step1 wouldn't have applied and we'd then be
looking at a MULT_EXPR not a WIDEN_MULT_EXPR.


Not necessarily.

For example, it will use a 32x32-64 signed widening multiply for 16 bit 
unsigned data if there is no unsigned 16x16-64 option. Hypothetically, 
if there happened to be an unsigned 16x16-64 multiply-and-accumulate 
then you'd end up masking it.


Like I said though, cross that bridge if it ever comes to it.

Andrew

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Richard Earnshaw]

On 17/08/12 15:39, Andrew Stubbs wrote:
 On 17/08/12 15:31, Richard Earnshaw wrote:
 On 17/08/12 15:22, Andrew Stubbs wrote:
 On 17/08/12 15:04, Richard Earnshaw wrote:
 The fix is to make is_widening_mult_p note that it has been called with
 a WIDEN_MULT_EXPR and rather than decompose the operands again, to
 simply extract the existing operands, which have already been formulated
 correctly for a widening multiply operation.

 As long as the existing test cases work, I think the only problem with
 this idea is if some architecture has a wider range of widening
 multiply-and-accumulate than it does plain widening multiply.

 But surely in that case step1 wouldn't have applied and we'd then be
 looking at a MULT_EXPR not a WIDEN_MULT_EXPR.
 
 Not necessarily.
 
 For example, it will use a 32x32-64 signed widening multiply for 16 bit 
 unsigned data if there is no unsigned 16x16-64 option. Hypothetically, 
 if there happened to be an unsigned 16x16-64 multiply-and-accumulate 
 then you'd end up masking it.
 
 Like I said though, cross that bridge if it ever comes to it.
 
 Andrew
 

You've lost me.

If we don't have a 16x16-64 mult operation then after step 1 we'll
still have a MULT_EXPR, not a WIDEN_MULT_EXPR, so when we reach step2
there's nothing to short circuit.

Unless, of course, you're expecting us to get

step1 - 16x16-32 widen mult
step2 - widen64(step1) + acc64

But even this is only safe iff widen64 is the same type of widening as
the original widening step, and we determine that by looking at the
operands of the widening mult, not at any casts on them.

The key thing is that the type of multiply that we want is based on the
types of the operands, not on the type of the result.  So it's essential
we don't strip type conversions beyond the widening conversion.

R.

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Andrew Stubbs]

On 17/08/12 15:47, Richard Earnshaw wrote:

If we don't have a 16x16-64 mult operation then after step 1 we'll
still have a MULT_EXPR, not a WIDEN_MULT_EXPR, so when we reach step2
there's nothing to short circuit.

Unless, of course, you're expecting us to get

step1 - 16x16-32 widen mult
step2 - widen64(step1) + acc64


No, given a u16xu16-u64 operation in the code, and that the arch 
doesn't have such an opcode, I'd expect to get


step1 - (u32)u16 x (u32)u16 - u64

Likewise, 8x8-32 might give (16)8x(16)8-32.

The code can't see that the widening operation is non-optimal without 
looking beyond into its inputs.


Andrew

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Richard Earnshaw]

On 17/08/12 16:06, Andrew Stubbs wrote:
 On 17/08/12 15:47, Richard Earnshaw wrote:
 If we don't have a 16x16-64 mult operation then after step 1 we'll
 still have a MULT_EXPR, not a WIDEN_MULT_EXPR, so when we reach step2
 there's nothing to short circuit.

 Unless, of course, you're expecting us to get

 step1 - 16x16-32 widen mult
 step2 - widen64(step1) + acc64
 
 No, given a u16xu16-u64 operation in the code, and that the arch 
 doesn't have such an opcode, I'd expect to get
 
 step1 - (u32)u16 x (u32)u16 - u64

Hmm, I would have thought that would be more costly than

(u64)(u16 x u16 - u32)

 
 Likewise, 8x8-32 might give (16)8x(16)8-32.
 
 The code can't see that the widening operation is non-optimal without 
 looking beyond into its inputs.

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.

R.

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Richard Earnshaw]

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.

--- 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
{

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Andrew Stubbs]

On 17/08/12 16:20, Richard Earnshaw wrote:

No, given a u16xu16-u64 operation in the code, and that the arch
doesn't have such an opcode, I'd expect to get

step1 - (u32)u16 x (u32)u16 - u64


Hmm, I would have thought that would be more costly than

(u64)(u16 x u16 - u32)


You might be right, but then extends are often free, especially with 
unsigned types, so it's hard to say for sure.


Did you reproduce one? It's a long time since I last looked at this 
stuff, so I could be confused.


Andrew

Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Andrew Stubbs]

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