Re: [PATCH (3/7)] Widening multiply-and-accumulate pattern matching

[Andrew Stubbs <ams at codesourcery dot com>]

On 07/07/11 10:58, Richard Guenther wrote:
> I think you should assume that series of widenings, (int)(short)char_variable
> are already combined.  Thus I believe you only need to consider a single
> conversion in valid_types_for_madd_p.

Hmm, I'm not so sure. I'll look into it a bit further.

> +/* Check the input types, TYPE1 and TYPE2 to a widening multiply,
>
> what are those types?  Is TYPE1 the result type and TYPE2 the
> operand type?  If so why

TYPE1 and TYPE2 are the inputs to the multiply. I thought I explained 
that in the comment before the function.

> +  initial_bitsize = TYPE_PRECISION (type1) + TYPE_PRECISION (type2);
>
> this?!

The result of the multiply will be this many bits wide. This may be 
narrower than the type that holds it.

E.g., 16-bit * 8-bit gives a result at most 24-bits wide, which will 
usually be held in a 32- or 64-bit variable.

> +  initial_unsigned = TYPE_UNSIGNED (type1)&&  TYPE_UNSIGNED (type2);
>
> that also looks odd.  So probably TYPE1 isn't the result type.  If they
> are the types of the operands, then what operand is EXPR for?

EXPR, as the comment says, is the addition that follows the multiply.

> -  if (TREE_CODE (rhs1) == SSA_NAME)
> +  for (tmp = rhs1, rhs1_code = ERROR_MARK;
> +       TREE_CODE (tmp) == SSA_NAME
> +&&  (CONVERT_EXPR_CODE_P (rhs1_code) || rhs1_code == ERROR_MARK);
> +       tmp = gimple_assign_rhs1 (rhs1_stmt))
>       {
> -      rhs1_stmt = SSA_NAME_DEF_STMT (rhs1);
> -      if (is_gimple_assign (rhs1_stmt))
> -       rhs1_code = gimple_assign_rhs_code (rhs1_stmt);
> +      rhs1_stmt = SSA_NAME_DEF_STMT (tmp);
> +      if (!is_gimple_assign (rhs1_stmt))
> +       break;
> +      rhs1_code = gimple_assign_rhs_code (rhs1_stmt);
>       }
>
> the result looks a bit like spaghetti code ... and lacks a comment
> on what it is trying to do.  It looks like it sees through an arbitrary
> number of conversions - possibly ones that will make the
> macc invalid, as for (short)int-var * short-var + int-var.  So you'll
> be pessimizing code by doing that unconditionally.  As I said
> above you should at most consider one intermediate conversion.

Ok, I need to add a comment here. The code does indeed look back through 
an arbitrary number of conversions. It is searching for the last real 
operation before the addition, hoping to find a multiply.

> I believe the code should be arranged such that only valid
> conversions are looked through in the first place.  Valid, in
> that the resulting types should still match the macc constraints.

Well, it might be possible to discard some conversions initially, but 
until the multiply is found, and it's input types are known, we can't 
know for certain what conversions are valid.

I think I need to explain what's going on here more clearly.

  1. It finds an addition statement. It's not known yet whether it is 
part of a multiply-and-accumulate, or not.

  2. It follows the conversion chain back from each operand to see if 
it finds a multiply, or widening multiply statement.

  3. If it finds a non-widening multiply, it checks it to see if it 
could be widening multiply-and-accumulate (it will already have been 
rejected as a widening multiply on it's own, but the addition might be 
in a wider mode, or the target might provide multiply-and-accumulate 
insns that don't have corresponding widening multiply insns).

  4. (This is the new bit!) It looks to see if there are any 
conversions between the multiply and addition that can safely be ignored.

  5. If we get here, then emit any necessary conversion statements, and 
convert the addition to a WIDEN_MULT_PLUS_EXPR.

Before these changes, any conversion between the multiply and addition 
statements would prevent optimization, even though there are many cases 
where the conversions are valid, and even inserted automatically.

I'm going to go away and find out whether there are really any cases 
where there can legitimately be more than one conversion, and at least 
update my patch with better commenting.

Thanks for you review.

Andrew