Tamar Christina <tamar.christ...@arm.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandif...@arm.com>
>> Sent: Monday, July 12, 2021 11:26 AM
>> To: Tamar Christina <tamar.christ...@arm.com>
>> Cc: Richard Biener <rguent...@suse.de>; nd <n...@arm.com>; gcc-
>> patc...@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.christ...@arm.com> writes:
>> >> -----Original Message-----
>> >> From: Richard Sandiford <richard.sandif...@arm.com>
>> >> Sent: Monday, July 12, 2021 10:39 AM
>> >> To: Tamar Christina <tamar.christ...@arm.com>
>> >> Cc: Richard Biener <rguent...@suse.de>; nd <n...@arm.com>; gcc-
>> >> patc...@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.christ...@arm.com> writes:
>> >> > Hi,
>> >> >
>> >> >> Richard Sandiford <richard.sandif...@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
