Re: [PATCH 1/4]middle-end Vect: Add support for dot-product where the sign for the multiplicant changes.

[Richard Sandiford <richard.sandiford@arm.com>]

Tamar Christina <Tamar.Christina@arm.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandiford@arm.com>
>> Sent: Monday, July 12, 2021 11:26 AM
>> To: Tamar Christina <Tamar.Christina@arm.com>
>> Cc: Richard Biener <rguenther@suse.de>; nd <nd@arm.com>; gcc-
>> patches@gcc.gnu.org
>> Subject: Re: [PATCH 1/4]middle-end Vect: Add support for dot-product
>> where the sign for the multiplicant changes.
>> 
>> Tamar Christina <Tamar.Christina@arm.com> writes:
>> >> -----Original Message-----
>> >> From: Richard Sandiford <richard.sandiford@arm.com>
>> >> Sent: Monday, July 12, 2021 10:39 AM
>> >> To: Tamar Christina <Tamar.Christina@arm.com>
>> >> Cc: Richard Biener <rguenther@suse.de>; nd <nd@arm.com>; gcc-
>> >> patches@gcc.gnu.org
>> >> Subject: Re: [PATCH 1/4]middle-end Vect: Add support for dot-product
>> >> where the sign for the multiplicant changes.
>> >>
>> >> Tamar Christina <Tamar.Christina@arm.com> writes:
>> >> > Hi,
>> >> >
>> >> >> Richard Sandiford <richard.sandiford@arm.com> writes:
>> >> >> >> @@ -992,21 +1029,27 @@ vect_recog_dot_prod_pattern (vec_info
>> >> >> *vinfo,
>> >> >> >>    /* FORNOW.  Can continue analyzing the def-use chain when
>> >> >> >> this stmt in
>> >> >> a phi
>> >> >> >>       inside the loop (in case we are analyzing an outer-loop).  */
>> >> >> >>    vect_unpromoted_value unprom0[2];
>> >> >> >> +  enum optab_subtype subtype = optab_vector;
>> >> >> >>    if (!vect_widened_op_tree (vinfo, mult_vinfo, MULT_EXPR,
>> >> >> WIDEN_MULT_EXPR,
>> >> >> >> -			     false, 2, unprom0, &half_type))
>> >> >> >> +			     false, 2, unprom0, &half_type, &subtype))
>> >> >> >> +    return NULL;
>> >> >> >> +
>> >> >> >> +  if (subtype == optab_vector_mixed_sign
>> >> >> >> +      && TYPE_UNSIGNED (unprom_mult.type)
>> >> >> >> +      && TYPE_PRECISION (half_type) * 4 > TYPE_PRECISION
>> >> >> >> + (unprom_mult.type))
>> >> >> >>      return NULL;
>> >> >> >
>> >> >> > Isn't the final condition here instead that TYPE1 is narrower than
>> TYPE2?
>> >> >> > I.e. we need to reject the case in which we multiply a signed
>> >> >> > and an unsigned value to get a (logically) signed result, but
>> >> >> > then zero-extend it (rather than sign-extend it) to the
>> >> >> > precision of the
>> >> addition.
>> >> >> >
>> >> >> > That would make the test:
>> >> >> >
>> >> >> >   if (subtype == optab_vector_mixed_sign
>> >> >> >       && TYPE_UNSIGNED (unprom_mult.type)
>> >> >> >       && TYPE_PRECISION (unprom_mult.type) < TYPE_PRECISION
>> (type))
>> >> >> >     return NULL;
>> >> >> >
>> >> >> > instead.
>> >> >>
>> >> >> And folding that into the existing test gives:
>> >> >>
>> >> >>   /* If there are two widening operations, make sure they agree on
>> >> >> the
>> >> sign
>> >> >>      of the extension.  The result of an optab_vector_mixed_sign
>> operation
>> >> >>      is signed; otherwise, the result has the same sign as the operands.
>> */
>> >> >>   if (TYPE_PRECISION (unprom_mult.type) != TYPE_PRECISION (type)
>> >> >>       && (subtype == optab_vector_mixed_sign
>> >> >> 	  ? TYPE_UNSIGNED (unprom_mult.type)
>> >> >> 	  : TYPE_SIGN (unprom_mult.type) != TYPE_SIGN (half_type)))
>> >> >>     return NULL;
>> >> >>
>> >> >
>> >> > I went with the first one which doesn't add the extra constraints
>> >> > for the normal dotproduct as that makes it too restrictive. It's
>> >> > the type of the multiplication that determines the operation so
>> >> > dotproduct can be used a bit more than where we currently do.
>> >> >
>> >> > This was relaxed in an earlier patch.
>> >>
>> >> I didn't mean that we should add extra constraints to the normal case
>> though.
>> >> The existing test I was referring to above was:
>> >>
>> >>   /* If there are two widening operations, make sure they agree on
>> >>      the sign of the extension.  */
>> >>   if (TYPE_PRECISION (unprom_mult.type) != TYPE_PRECISION (type)
>> >>       && TYPE_SIGN (unprom_mult.type) != TYPE_SIGN (half_type))
>> >>     return NULL;
>> >
>> > But as I mentioned, this restriction is unneeded and has been removed
>> hence why it's not in my patchset's diff.
>> > It's removed by
>> > https://gcc.gnu.org/pipermail/gcc-patches/2021-May/569851.html which
>> Richi conditioned on the rest of these patches being approved.
>> >
>> > This change needlessly blocks test vect-reduc-dot-[2,3,6,7].c from
>> > being dotproducts for instance
>> >
>> > It's also part of the deficiency between GCC codegen and Clang
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88492#c6
>> 
>> Hmm, OK.  Just removing the check regresses:
>> 
>> unsigned long __attribute__ ((noipa))
>> f (signed short *x, signed short *y)
>> {
>>   unsigned long res = 0;
>>   for (int i = 0; i < 100; ++i)
>>     res += (unsigned int) x[i] * (unsigned int) y[i];
>>   return res;
>> }
>> 
>> int
>> main (void)
>> {
>>   signed short x[100], y[100];
>>   for (int i = 0; i < 100; ++i)
>>     {
>>       x[i] = -1;
>>       y[i] = 1;
>>     }
>>   if (f (x, y) != 0x6400000000ULL - 100)
>>     __builtin_abort ();
>>   return 0;
>> }
>> 
>> on SVE.  We then use SDOT even though the result of the multiplication is
>> zero- rather than sign-extended to 64 bits.  Does something else in the series
>> stop that from that happening?
>
> No, and I hadn't noticed it before because it looks like the mid-end tests that are execution test don't turn on dot-product for arm targets :/ 

Yeah, I was surprised I needed SVE to get an SDOT above, but didn't look
into why…

> I'll look at it separately, for now I've then added the check back in.
>
> Ok for trunk now?

Reviewing the full patch this time: I have a couple of nits about
the documentation, but otherwise it LGTM.

> --- a/gcc/doc/md.texi
> +++ b/gcc/doc/md.texi
> @@ -5446,13 +5446,55 @@ Like @samp{fold_left_plus_@var{m}}, but takes an additional mask operand
>  
>  @cindex @code{sdot_prod@var{m}} instruction pattern
>  @item @samp{sdot_prod@var{m}}
> +
> +Compute the sum of the products of two signed elements.
> +Operand 1 and operand 2 are of the same mode. Their
> +product, which is of a wider mode, is computed and added to operand 3.
> +Operand 3 is of a mode equal or wider than the mode of the product. The
> +result is placed in operand 0, which is of the same mode as operand 3.
> +
> +Semantically the expressions perform the multiplication in the following signs
> +
> +@smallexample
> +sdot<signed c, signed a, signed b> ==
> +   res = sign-ext (a) * sign-ext (b) + c
> +@dots{}
> +@end smallexample

I think putting signed c first in the argument list might be confusing,
since like you say, it corresponds to operand 3 rather than operand 1.
How about calling them op0, op1, op2 and op3 instead of res, a, b and c,
and listing them in that order?

Same for udot_prod.

(Someone who doesn't know the AArch64 instructions might wonder how
the elements of op1 and op2 correspond to elements of op0 and op3.
That's a pre-existing problem though, so no need to fix it here.)

>  @cindex @code{udot_prod@var{m}} instruction pattern
> -@itemx @samp{udot_prod@var{m}}
> -Compute the sum of the products of two signed/unsigned elements.
> -Operand 1 and operand 2 are of the same mode. Their product, which is of a
> -wider mode, is computed and added to operand 3. Operand 3 is of a mode equal or
> -wider than the mode of the product. The result is placed in operand 0, which
> -is of the same mode as operand 3.
> +@item @samp{udot_prod@var{m}}
> +
> +Compute the sum of the products of two unsigned elements.
> +Operand 1 and operand 2 are of the same mode. Their
> +product, which is of a wider mode, is computed and added to operand 3.
> +Operand 3 is of a mode equal or wider than the mode of the product. The
> +result is placed in operand 0, which is of the same mode as operand 3.
> +
> +Semantically the expressions perform the multiplication in the following signs
> +
> +@smallexample
> +udot<unsigned c, unsigned a, unsigned b> ==
> +   res = zero-ext (a) * zero-ext (b) + c
> +@dots{}
> +@end smallexample
> +
> +
> +

Should just be one blank line here.

> +@cindex @code{usdot_prod@var{m}} instruction pattern
> +@item @samp{usdot_prod@var{m}}
> +Compute the sum of the products of elements of different signs.
> +Operand 1 must be unsigned and operand 2 signed. Their
> +product, which is of a wider mode, is computed and added to operand 3.
> +Operand 3 is of a mode equal or wider than the mode of the product. The
> +result is placed in operand 0, which is of the same mode as operand 3.
> +
> +Semantically the expressions perform the multiplication in the following signs
> +
> +@smallexample
> +usdot<unsigned c, unsigned a, signed b> ==
> +   res = ((unsigned-conv) sign-ext (a)) * zero-ext (b) + c

It looks like the extensions are the wrong way around.  I think it should be:

usdot<signed c, unsigned a, signed b> ==
   res = ((signed-conv) zero-ext (a)) * sign-ext (b) + c

(before the changes to put c last and use the opN names).

I.e. the unsigned operand is zero-extended and the signed operand is
sign extended.  I think it's easier to understand if we treat the
multiplication and c as signed, since in that case we don't reinterpret
any negative signed value (of b) as an unsigned value.  (Both choices
make sense for “a”, since the zero-ext(a) fits into both a signed wider
int and an unsigned wider int.)

OK with those changes, and thanks for your patience through the slow reviews.

Richard