Tamar Christina <tamar.christ...@arm.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandif...@arm.com>
>> Sent: Thursday, July 15, 2021 8:35 PM
>> To: Tamar Christina <tamar.christ...@arm.com>
>> Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; Richard Earnshaw
>> <richard.earns...@arm.com>; Marcus Shawcroft
>> <marcus.shawcr...@arm.com>; Kyrylo Tkachov <kyrylo.tkac...@arm.com>
>> Subject: Re: [PATCH 2/4]AArch64: correct usdot vectorizer and intrinsics
>> optabs
>>
>> Tamar Christina <tamar.christ...@arm.com> writes:
>> > Hi All,
>> >
>> > There's a slight mismatch between the vectorizer optabs and the
>> > intrinsics patterns for NEON. The vectorizer expects operands[3] and
>> > operands[0] to be the same but the aarch64 intrinsics expanders expect
>> > operands[0] and operands[1] to be the same.
>> >
>> > This means we need different patterns here. This adds a separate
>> > usdot vectorizer pattern which just shuffles around the RTL params.
>> >
>> > There's also an inconsistency between the usdot and (u|s)dot
>> > intrinsics RTL patterns which is not corrected here.
>> >
>> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>> >
>> > Ok for master?
>>
>> Couldn't we just change:
>>
>> > diff --git a/gcc/config/aarch64/arm_neon.h
>> > b/gcc/config/aarch64/arm_neon.h index
>> >
>> 00d76ea937ace5763746478cbdfadf6479e0b15a..17e059efb80fa86a8a32127ac
>> e4f
>> > c7f43e2040a8 100644
>> > --- a/gcc/config/aarch64/arm_neon.h
>> > +++ b/gcc/config/aarch64/arm_neon.h
>> > @@ -34039,14 +34039,14 @@ __extension__ extern __inline int32x2_t
>> > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
>> > vusdot_s32 (int32x2_t __r, uint8x8_t __a, int8x8_t __b) {
>> > - return __builtin_aarch64_usdot_prodv8qi_ssus (__r, __a, __b);
>> > + return __builtin_aarch64_usdotv8qi_ssus (__r, __a, __b);
>>
>> …this to __builtin_aarch64_usdot_prodv8qi_ssus (__a, __b, __r) etc.?
>
> Not easily, as I was mentioning before, Neon intrinsics have the assumption
> that
> operands[0] and operands[1] are the same. And this goes much further than just
> the header call.
>
> The actual type is determined by the optabs and the C stubs that are
> generated.
>
> aarch64_init_simd_builtins which creates the C function stubs starts
> processing
> arguments from the end and on non-void functions assumes that the value at
> operands[0] be the return type. So simply moving __r will get it to think that
> the result type should be uint8x8_t.
Yeah, the mode of operand 0 (i.e. the output) determines the return type.
But that mode isn't changing, so the return type will be correct for both
input operand orders. It works for me locally with:
diff --git a/gcc/config/aarch64/aarch64-simd.md
b/gcc/config/aarch64/aarch64-simd.md
index 88fa5ba5a44..5987d9af7c6 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -610,12 +610,12 @@ (define_expand "cmul<conj_op><mode>3"
;; and so the vectorizer provides r, in which the result has to be accumulated.
(define_insn "<sur>dot_prod<vsi2qi>"
[(set (match_operand:VS 0 "register_operand" "=w")
- (plus:VS (match_operand:VS 1 "register_operand" "0")
- (unspec:VS [(match_operand:<VSI2QI> 2 "register_operand" "w")
- (match_operand:<VSI2QI> 3 "register_operand" "w")]
- DOTPROD)))]
+ (plus:VS (unspec:VS [(match_operand:<VSI2QI> 1 "register_operand" "w")
+ (match_operand:<VSI2QI> 2 "register_operand" "w")]
+ DOTPROD)
+ (match_operand:VS 3 "register_operand" "0")))]
"TARGET_DOTPROD"
- "<sur>dot\\t%0.<Vtype>, %2.<Vdottype>, %3.<Vdottype>"
+ "<sur>dot\\t%0.<Vtype>, %1.<Vdottype>, %2.<Vdottype>"
[(set_attr "type" "neon_dot<q>")]
)
diff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h
index 597f44ce106..64b6d43a1a0 100644
--- a/gcc/config/aarch64/arm_neon.h
+++ b/gcc/config/aarch64/arm_neon.h
@@ -31767,28 +31767,28 @@ __extension__ extern __inline uint32x2_t
__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
vdot_u32 (uint32x2_t __r, uint8x8_t __a, uint8x8_t __b)
{
- return __builtin_aarch64_udot_prodv8qi_uuuu (__r, __a, __b);
+ return __builtin_aarch64_udot_prodv8qi_uuuu (__a, __b, __r);
}
__extension__ extern __inline uint32x4_t
__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
vdotq_u32 (uint32x4_t __r, uint8x16_t __a, uint8x16_t __b)
{
- return __builtin_aarch64_udot_prodv16qi_uuuu (__r, __a, __b);
+ return __builtin_aarch64_udot_prodv16qi_uuuu (__a, __b, __r);
}
__extension__ extern __inline int32x2_t
__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
vdot_s32 (int32x2_t __r, int8x8_t __a, int8x8_t __b)
{
- return __builtin_aarch64_sdot_prodv8qi (__r, __a, __b);
+ return __builtin_aarch64_sdot_prodv8qi (__a, __b, __r);
}
__extension__ extern __inline int32x4_t
__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
vdotq_s32 (int32x4_t __r, int8x16_t __a, int8x16_t __b)
{
- return __builtin_aarch64_sdot_prodv16qi (__r, __a, __b);
+ return __builtin_aarch64_sdot_prodv16qi (__a, __b, __r);
}
__extension__ extern __inline uint32x2_t
Thanks,
Richard
