* [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns
@ 2018-07-11 10:36 Matthew Malcomson
2018-07-12 10:18 ` Richard Sandiford
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Malcomson @ 2018-07-11 10:36 UTC (permalink / raw)
To: gcc-patches; +Cc: richard.earnshaw, james.greenhalgh, marcus.shawcroft, nd
[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]
Hi there,
The current RTL patterns for widening addition and subtraction
instructions in
aarch64-simd.md use the code iterator attribute <ADDSUB:optab> to make their
definition more compact.
This approach means that the `minus` and `plus` cases have their operands in
the same order, which causes problems in matching.
The `minus` case needs the more complex operand second to be semantically
correct, but the `plus` case needs the more complex operand first to be in
canonical form.
This patch splits the RTL patterns into two, one for `plus` and one for
`minus` with differing operand order to match their differing requirements.
Ready for trunk?
Bootstrap and test on aarch64-none-linux-gnu
Changelog for gcc/testsuite/Changelog
2018-07-10 Matthew Malcomson <matthew.malcomson@arm.com>
   * gcc.target/aarch64/vect-su-add-sub.c: New.
Changelog for gcc/Changelog
2018-07-10 Matthew Malcomson <matthew.malcomson@arm.com>
   * config/aarch64/aarch64-simd.md
(aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>): Split into...
   (aarch64_<ANY_EXTEND:su>subw<mode>): ... This...
   (aarch64_<ANY_EXTEND:su>addw<mode>): ... And this.
(aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>_internal): Split into...
   (aarch64_<ANY_EXTEND:su>subw<mode>_internal): ... This...
   (aarch64_<ANY_EXTEND:su>addw<mode>_internal): ... And this.
(aarch64_<ANY_EXTEND:su><ADDSUB:optab>w2<mode>_internal): Split into...
   (aarch64_<ANY_EXTEND:su>subw2<mode>_internal): ... This...
   (aarch64_<ANY_EXTEND:su>addw2<mode>_internal): ... And this.
[-- Attachment #2: canonicalize-aarch64-widening-plus-simd.patch --]
[-- Type: text/x-patch, Size: 5968 bytes --]
diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index aac5fa146ed8dde4507a0eb4ad6a07ce78d2f0cd..67b29cbe2cad91e031ee23be656ec61a403f2cf9 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -3302,38 +3302,78 @@
DONE;
})
-(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
+(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
[(set (match_operand:<VWIDE> 0 "register_operand" "=w")
- (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
- (ANY_EXTEND:<VWIDE>
- (match_operand:VD_BHSI 2 "register_operand" "w"))))]
+ (minus:<VWIDE>
+ (match_operand:<VWIDE> 1 "register_operand" "w")
+ (ANY_EXTEND:<VWIDE>
+ (match_operand:VD_BHSI 2 "register_operand" "w"))))]
"TARGET_SIMD"
- "<ANY_EXTEND:su><ADDSUB:optab>w\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
- [(set_attr "type" "neon_<ADDSUB:optab>_widen")]
+ "<ANY_EXTEND:su>subw\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
+ [(set_attr "type" "neon_sub_widen")]
)
-(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>_internal"
+(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>_internal"
[(set (match_operand:<VWIDE> 0 "register_operand" "=w")
- (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
- (ANY_EXTEND:<VWIDE>
- (vec_select:<VHALF>
- (match_operand:VQW 2 "register_operand" "w")
- (match_operand:VQW 3 "vect_par_cnst_lo_half" "")))))]
+ (minus:<VWIDE>
+ (match_operand:<VWIDE> 1 "register_operand" "w")
+ (ANY_EXTEND:<VWIDE>
+ (vec_select:<VHALF>
+ (match_operand:VQW 2 "register_operand" "w")
+ (match_operand:VQW 3 "vect_par_cnst_lo_half" "")))))]
"TARGET_SIMD"
- "<ANY_EXTEND:su><ADDSUB:optab>w\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vhalftype>"
- [(set_attr "type" "neon_<ADDSUB:optab>_widen")]
+ "<ANY_EXTEND:su>subw\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vhalftype>"
+ [(set_attr "type" "neon_sub_widen")]
)
-(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w2<mode>_internal"
+(define_insn "aarch64_<ANY_EXTEND:su>subw2<mode>_internal"
[(set (match_operand:<VWIDE> 0 "register_operand" "=w")
- (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
- (ANY_EXTEND:<VWIDE>
- (vec_select:<VHALF>
- (match_operand:VQW 2 "register_operand" "w")
- (match_operand:VQW 3 "vect_par_cnst_hi_half" "")))))]
+ (minus:<VWIDE>
+ (match_operand:<VWIDE> 1 "register_operand" "w")
+ (ANY_EXTEND:<VWIDE>
+ (vec_select:<VHALF>
+ (match_operand:VQW 2 "register_operand" "w")
+ (match_operand:VQW 3 "vect_par_cnst_hi_half" "")))))]
+ "TARGET_SIMD"
+ "<ANY_EXTEND:su>subw2\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
+ [(set_attr "type" "neon_sub_widen")]
+)
+
+(define_insn "aarch64_<ANY_EXTEND:su>addw<mode>"
+ [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
+ (plus:<VWIDE>
+ (ANY_EXTEND:<VWIDE>
+ (match_operand:VD_BHSI 2 "register_operand" "w"))
+ (match_operand:<VWIDE> 1 "register_operand" "w")))]
"TARGET_SIMD"
- "<ANY_EXTEND:su><ADDSUB:optab>w2\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
- [(set_attr "type" "neon_<ADDSUB:optab>_widen")]
+ "<ANY_EXTEND:su>addw\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
+ [(set_attr "type" "neon_add_widen")]
+)
+
+(define_insn "aarch64_<ANY_EXTEND:su>addw<mode>_internal"
+ [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
+ (plus:<VWIDE>
+ (ANY_EXTEND:<VWIDE>
+ (vec_select:<VHALF>
+ (match_operand:VQW 2 "register_operand" "w")
+ (match_operand:VQW 3 "vect_par_cnst_lo_half" "")))
+ (match_operand:<VWIDE> 1 "register_operand" "w")))]
+ "TARGET_SIMD"
+ "<ANY_EXTEND:su>addw\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vhalftype>"
+ [(set_attr "type" "neon_add_widen")]
+)
+
+(define_insn "aarch64_<ANY_EXTEND:su>addw2<mode>_internal"
+ [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
+ (plus:<VWIDE>
+ (ANY_EXTEND:<VWIDE>
+ (vec_select:<VHALF>
+ (match_operand:VQW 2 "register_operand" "w")
+ (match_operand:VQW 3 "vect_par_cnst_hi_half" "")))
+ (match_operand:<VWIDE> 1 "register_operand" "w")))]
+ "TARGET_SIMD"
+ "<ANY_EXTEND:su>addw2\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
+ [(set_attr "type" "neon_add_widen")]
)
(define_expand "aarch64_saddw2<mode>"
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/vect_su_add_sub.c b/gcc/testsuite/gcc.target/aarch64/simd/vect_su_add_sub.c
new file mode 100644
index 0000000000000000000000000000000000000000..15956ed83fdd5fc8dc895ab1ac4de3f98bc8a625
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/simd/vect_su_add_sub.c
@@ -0,0 +1,56 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+/* Ensure we use the signed/unsigned extend vectorized add and sub
+ instructions. */
+#define N 1024
+
+int a[N];
+long c[N];
+long d[N];
+unsigned int ua[N];
+unsigned long uc[N];
+unsigned long ud[N];
+
+void
+add ()
+{
+ for (int i = 0; i < N; i++)
+ d[i] = a[i] + c[i];
+}
+
+void
+subtract ()
+{
+ for (int i = 0; i < N; i++)
+ d[i] = c[i] - a[i];
+}
+
+void
+uadd ()
+{
+ for (int i = 0; i < N; i++)
+ ud[i] = ua[i] + uc[i];
+}
+
+void
+usubtract ()
+{
+ for (int i = 0; i < N; i++)
+ ud[i] = uc[i] - ua[i];
+}
+
+/* Ensure
+ saddw2 and one saddw for the function add()
+ ssubw2 and one ssubw for the function subtract()
+ uaddw2 and one uaddw for the function uadd()
+ usubw2 and one usubw for the function usubtract() */
+
+/* { dg-final { scan-assembler-times "\[ \t\]ssubw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]ssubw\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]saddw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]saddw\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]usubw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]usubw\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]uaddw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]uaddw\[ \t\]+" 1 } } */
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns
2018-07-11 10:36 [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns Matthew Malcomson
@ 2018-07-12 10:18 ` Richard Sandiford
2018-07-12 10:39 ` Sudakshina Das
0 siblings, 1 reply; 8+ messages in thread
From: Richard Sandiford @ 2018-07-12 10:18 UTC (permalink / raw)
To: Matthew Malcomson
Cc: gcc-patches, richard.earnshaw, james.greenhalgh, marcus.shawcroft, nd
Looks good to me FWIW (not a maintainer), just a minor formatting thing:
Matthew Malcomson <matthew.malcomson@arm.com> writes:
> diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
> index aac5fa146ed8dde4507a0eb4ad6a07ce78d2f0cd..67b29cbe2cad91e031ee23be656ec61a403f2cf9 100644
> --- a/gcc/config/aarch64/aarch64-simd.md
> +++ b/gcc/config/aarch64/aarch64-simd.md
> @@ -3302,38 +3302,78 @@
> DONE;
> })
>
> -(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
> +(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> - (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
> - (ANY_EXTEND:<VWIDE>
> - (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> + (minus:<VWIDE>
> + (match_operand:<VWIDE> 1 "register_operand" "w")
> + (ANY_EXTEND:<VWIDE>
> + (match_operand:VD_BHSI 2 "register_operand" "w"))))]
The (minus should be under the "(match_operand":
(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
[(set (match_operand:<VWIDE> 0 "register_operand" "=w")
(minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
(ANY_EXTEND:<VWIDE>
(match_operand:VD_BHSI 2 "register_operand" "w"))))]
Same for the other patterns.
Thanks,
Richard
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns
2018-07-12 10:18 ` Richard Sandiford
@ 2018-07-12 10:39 ` Sudakshina Das
2018-07-19 12:35 ` Matthew Malcomson
0 siblings, 1 reply; 8+ messages in thread
From: Sudakshina Das @ 2018-07-12 10:39 UTC (permalink / raw)
To: Matthew Malcomson, gcc-patches, richard.earnshaw,
james.greenhalgh, marcus.shawcroft, richard.sandiford
Cc: nd
Hi Matthew
On 12/07/18 11:18, Richard Sandiford wrote:
> Looks good to me FWIW (not a maintainer), just a minor formatting thing:
>
> Matthew Malcomson <matthew.malcomson@arm.com> writes:
>> diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
>> index aac5fa146ed8dde4507a0eb4ad6a07ce78d2f0cd..67b29cbe2cad91e031ee23be656ec61a403f2cf9 100644
>> --- a/gcc/config/aarch64/aarch64-simd.md
>> +++ b/gcc/config/aarch64/aarch64-simd.md
>> @@ -3302,38 +3302,78 @@
>> DONE;
>> })
>>
>> -(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
>> +(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
>> [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
>> - (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
>> - (ANY_EXTEND:<VWIDE>
>> - (match_operand:VD_BHSI 2 "register_operand" "w"))))]
>> + (minus:<VWIDE>
>> + (match_operand:<VWIDE> 1 "register_operand" "w")
>> + (ANY_EXTEND:<VWIDE>
>> + (match_operand:VD_BHSI 2 "register_operand" "w"))))]
>
> The (minus should be under the "(match_operand":
>
> (define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
> (ANY_EXTEND:<VWIDE>
> (match_operand:VD_BHSI 2 "register_operand" "w"))))]
>
> Same for the other patterns.
>
> Thanks,
> Richard
>
You will need a maintainer's approval but this looks good to me.
Thanks for doing this. I would only point out one other nit which you
can choose to ignore:
+/* Ensure
+ saddw2 and one saddw for the function add()
+ ssubw2 and one ssubw for the function subtract()
+ uaddw2 and one uaddw for the function uadd()
+ usubw2 and one usubw for the function usubtract() */
+
+/* { dg-final { scan-assembler-times "\[ \t\]ssubw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]ssubw\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]saddw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]saddw\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]usubw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]usubw\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]uaddw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]uaddw\[ \t\]+" 1 } } */
The scan-assembly directives for the different
functions can be placed right below each of them and that would
make it easier to read the expected results in the test and you
can get rid of the comments saying the same.
Thanks
Sudi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns
2018-07-12 10:39 ` Sudakshina Das
@ 2018-07-19 12:35 ` Matthew Malcomson
2018-07-24 15:12 ` James Greenhalgh
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Malcomson @ 2018-07-19 12:35 UTC (permalink / raw)
To: Sudakshina Das, gcc-patches, richard.earnshaw, james.greenhalgh,
marcus.shawcroft, richard.sandiford
Cc: nd
[-- Attachment #1: Type: text/plain, Size: 3292 bytes --]
Hi again.
Providing an updated patch to include the formatting suggestions.
Thanks,
Matthew
On 12/07/18 11:39, Sudakshina Das wrote:
> Hi Matthew
>
> On 12/07/18 11:18, Richard Sandiford wrote:
>> Looks good to me FWIW (not a maintainer), just a minor formatting thing:
>>
>> Matthew Malcomson <matthew.malcomson@arm.com> writes:
>>> diff --git a/gcc/config/aarch64/aarch64-simd.md
>>> b/gcc/config/aarch64/aarch64-simd.md
>>> index
>>> aac5fa146ed8dde4507a0eb4ad6a07ce78d2f0cd..67b29cbe2cad91e031ee23be656ec61a403f2cf9
>>> 100644
>>> --- a/gcc/config/aarch64/aarch64-simd.md
>>> +++ b/gcc/config/aarch64/aarch64-simd.md
>>> @@ -3302,38 +3302,78 @@
>>> Â Â Â DONE;
>>> Â })
>>> Â -(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
>>> +(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
>>> Â Â Â [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
>>> -Â Â Â Â Â Â Â (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand"
>>> "w")
>>> -Â Â Â Â Â Â Â Â Â Â Â (ANY_EXTEND:<VWIDE>
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â (match_operand:VD_BHSI 2 "register_operand" "w"))))]
>>> +Â Â Â Â Â Â Â Â Â Â Â (minus:<VWIDE>
>>> +Â Â Â Â Â Â Â Â Â Â Â Â (match_operand:<VWIDE> 1 "register_operand" "w")
>>> +Â Â Â Â Â Â Â Â Â Â Â Â (ANY_EXTEND:<VWIDE>
>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â (match_operand:VD_BHSI 2 "register_operand" "w"))))]
>>
>> The (minus should be under the "(match_operand":
>>
>> (define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
>> Â Â [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
>> Â Â Â Â (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â (ANY_EXTEND:<VWIDE>
>> Â Â Â Â Â Â Â Â Â Â Â Â (match_operand:VD_BHSI 2 "register_operand" "w"))))]
>>
>> Same for the other patterns.
>>
>> Thanks,
>> Richard
>>
>
> You will need a maintainer's approval but this looks good to me.
> Thanks for doing this. I would only point out one other nit which you
> can choose to ignore:
>
> +/* Ensure
> +Â Â saddw2 and one saddw for the function add()
> +Â Â ssubw2 and one ssubw for the function subtract()
> +Â Â uaddw2 and one uaddw for the function uadd()
> +Â Â usubw2 and one usubw for the function usubtract() */
> +
> +/* { dg-final { scan-assembler-times "\[ \t\]ssubw2\[ \t\]+" 1 } } */
> +/* { dg-final { scan-assembler-times "\[ \t\]ssubw\[ \t\]+" 1 } } */
> +/* { dg-final { scan-assembler-times "\[ \t\]saddw2\[ \t\]+" 1 } } */
> +/* { dg-final { scan-assembler-times "\[ \t\]saddw\[ \t\]+" 1 } } */
> +/* { dg-final { scan-assembler-times "\[ \t\]usubw2\[ \t\]+" 1 } } */
> +/* { dg-final { scan-assembler-times "\[ \t\]usubw\[ \t\]+" 1 } } */
> +/* { dg-final { scan-assembler-times "\[ \t\]uaddw2\[ \t\]+" 1 } } */
> +/* { dg-final { scan-assembler-times "\[ \t\]uaddw\[ \t\]+" 1 } } */
>
> The scan-assembly directives for the different
> functions can be placed right below each of them and that would
> make it easier to read the expected results in the test and you
> can get rid of the comments saying the same.
>
> Thanks
> Sudi
[-- Attachment #2: canonicalize-aarch64-widening-plus-simd.patch --]
[-- Type: text/x-patch, Size: 5728 bytes --]
diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index b5c551ad650e1a83416d5fbbbdd38e3fa3beb532..1f356d04d5b3542ac9ce51bc315c81d1fff91f21 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -3303,38 +3303,74 @@
DONE;
})
-(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
+(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
[(set (match_operand:<VWIDE> 0 "register_operand" "=w")
- (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
- (ANY_EXTEND:<VWIDE>
- (match_operand:VD_BHSI 2 "register_operand" "w"))))]
+ (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
+ (ANY_EXTEND:<VWIDE>
+ (match_operand:VD_BHSI 2 "register_operand" "w"))))]
"TARGET_SIMD"
- "<ANY_EXTEND:su><ADDSUB:optab>w\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
- [(set_attr "type" "neon_<ADDSUB:optab>_widen")]
+ "<ANY_EXTEND:su>subw\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
+ [(set_attr "type" "neon_sub_widen")]
)
-(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>_internal"
+(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>_internal"
[(set (match_operand:<VWIDE> 0 "register_operand" "=w")
- (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
- (ANY_EXTEND:<VWIDE>
- (vec_select:<VHALF>
- (match_operand:VQW 2 "register_operand" "w")
- (match_operand:VQW 3 "vect_par_cnst_lo_half" "")))))]
+ (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
+ (ANY_EXTEND:<VWIDE>
+ (vec_select:<VHALF>
+ (match_operand:VQW 2 "register_operand" "w")
+ (match_operand:VQW 3 "vect_par_cnst_lo_half" "")))))]
"TARGET_SIMD"
- "<ANY_EXTEND:su><ADDSUB:optab>w\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vhalftype>"
- [(set_attr "type" "neon_<ADDSUB:optab>_widen")]
+ "<ANY_EXTEND:su>subw\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vhalftype>"
+ [(set_attr "type" "neon_sub_widen")]
)
-(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w2<mode>_internal"
+(define_insn "aarch64_<ANY_EXTEND:su>subw2<mode>_internal"
[(set (match_operand:<VWIDE> 0 "register_operand" "=w")
- (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
- (ANY_EXTEND:<VWIDE>
- (vec_select:<VHALF>
- (match_operand:VQW 2 "register_operand" "w")
- (match_operand:VQW 3 "vect_par_cnst_hi_half" "")))))]
+ (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
+ (ANY_EXTEND:<VWIDE>
+ (vec_select:<VHALF>
+ (match_operand:VQW 2 "register_operand" "w")
+ (match_operand:VQW 3 "vect_par_cnst_hi_half" "")))))]
"TARGET_SIMD"
- "<ANY_EXTEND:su><ADDSUB:optab>w2\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
- [(set_attr "type" "neon_<ADDSUB:optab>_widen")]
+ "<ANY_EXTEND:su>subw2\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
+ [(set_attr "type" "neon_sub_widen")]
+)
+
+(define_insn "aarch64_<ANY_EXTEND:su>addw<mode>"
+ [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
+ (plus:<VWIDE>
+ (ANY_EXTEND:<VWIDE> (match_operand:VD_BHSI 2 "register_operand" "w"))
+ (match_operand:<VWIDE> 1 "register_operand" "w")))]
+ "TARGET_SIMD"
+ "<ANY_EXTEND:su>addw\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
+ [(set_attr "type" "neon_add_widen")]
+)
+
+(define_insn "aarch64_<ANY_EXTEND:su>addw<mode>_internal"
+ [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
+ (plus:<VWIDE>
+ (ANY_EXTEND:<VWIDE>
+ (vec_select:<VHALF>
+ (match_operand:VQW 2 "register_operand" "w")
+ (match_operand:VQW 3 "vect_par_cnst_lo_half" "")))
+ (match_operand:<VWIDE> 1 "register_operand" "w")))]
+ "TARGET_SIMD"
+ "<ANY_EXTEND:su>addw\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vhalftype>"
+ [(set_attr "type" "neon_add_widen")]
+)
+
+(define_insn "aarch64_<ANY_EXTEND:su>addw2<mode>_internal"
+ [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
+ (plus:<VWIDE>
+ (ANY_EXTEND:<VWIDE>
+ (vec_select:<VHALF>
+ (match_operand:VQW 2 "register_operand" "w")
+ (match_operand:VQW 3 "vect_par_cnst_hi_half" "")))
+ (match_operand:<VWIDE> 1 "register_operand" "w")))]
+ "TARGET_SIMD"
+ "<ANY_EXTEND:su>addw2\\t%0.<Vwtype>, %1.<Vwtype>, %2.<Vtype>"
+ [(set_attr "type" "neon_add_widen")]
)
(define_expand "aarch64_saddw2<mode>"
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/vect_su_add_sub.c b/gcc/testsuite/gcc.target/aarch64/simd/vect_su_add_sub.c
new file mode 100644
index 0000000000000000000000000000000000000000..338da54f6281c90e1c6b1c59fa50d9b719005c77
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/simd/vect_su_add_sub.c
@@ -0,0 +1,49 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+/* Ensure we use the signed/unsigned extend vectorized add and sub
+ instructions. */
+#define N 1024
+
+int a[N];
+long c[N];
+long d[N];
+unsigned int ua[N];
+unsigned long uc[N];
+unsigned long ud[N];
+
+void
+add ()
+{
+ for (int i = 0; i < N; i++)
+ d[i] = a[i] + c[i];
+}
+/* { dg-final { scan-assembler-times "\[ \t\]saddw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]saddw\[ \t\]+" 1 } } */
+
+void
+subtract ()
+{
+ for (int i = 0; i < N; i++)
+ d[i] = c[i] - a[i];
+}
+/* { dg-final { scan-assembler-times "\[ \t\]ssubw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]ssubw\[ \t\]+" 1 } } */
+
+void
+uadd ()
+{
+ for (int i = 0; i < N; i++)
+ ud[i] = ua[i] + uc[i];
+}
+/* { dg-final { scan-assembler-times "\[ \t\]uaddw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]uaddw\[ \t\]+" 1 } } */
+
+void
+usubtract ()
+{
+ for (int i = 0; i < N; i++)
+ ud[i] = uc[i] - ua[i];
+}
+/* { dg-final { scan-assembler-times "\[ \t\]usubw2\[ \t\]+" 1 } } */
+/* { dg-final { scan-assembler-times "\[ \t\]usubw\[ \t\]+" 1 } } */
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns
2018-07-19 12:35 ` Matthew Malcomson
@ 2018-07-24 15:12 ` James Greenhalgh
2018-07-24 15:39 ` Kyrill Tkachov
0 siblings, 1 reply; 8+ messages in thread
From: James Greenhalgh @ 2018-07-24 15:12 UTC (permalink / raw)
To: Matthew Malcomson
Cc: Sudakshina Das, gcc-patches, Richard Earnshaw, Marcus Shawcroft,
Richard Sandiford, nd
On Thu, Jul 19, 2018 at 07:35:22AM -0500, Matthew Malcomson wrote:
> Hi again.
>
> Providing an updated patch to include the formatting suggestions.
Please try not to top-post replies, it makes the conversation thread
harder to follow (reply continues below!).
> On 12/07/18 11:39, Sudakshina Das wrote:
> > Hi Matthew
> >
> > On 12/07/18 11:18, Richard Sandiford wrote:
> >> Looks good to me FWIW (not a maintainer), just a minor formatting thing:
> >>
> >> Matthew Malcomson <matthew.malcomson@arm.com> writes:
> >>> diff --git a/gcc/config/aarch64/aarch64-simd.md
> >>> b/gcc/config/aarch64/aarch64-simd.md
> >>> index
> >>> aac5fa146ed8dde4507a0eb4ad6a07ce78d2f0cd..67b29cbe2cad91e031ee23be656ec61a403f2cf9
> >>> 100644
> >>> --- a/gcc/config/aarch64/aarch64-simd.md
> >>> +++ b/gcc/config/aarch64/aarch64-simd.md
> >>> @@ -3302,38 +3302,78 @@
> >>> Â Â Â DONE;
> >>> Â })
> >>> Â -(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
> >>> +(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> >>> Â Â Â [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> >>> -Â Â Â Â Â Â Â (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand"
> >>> "w")
> >>> -Â Â Â Â Â Â Â Â Â Â Â (ANY_EXTEND:<VWIDE>
> >>> -Â Â Â Â Â Â Â Â Â Â Â Â Â (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> >>> +Â Â Â Â Â Â Â Â Â Â Â (minus:<VWIDE>
> >>> +Â Â Â Â Â Â Â Â Â Â Â Â (match_operand:<VWIDE> 1 "register_operand" "w")
> >>> +Â Â Â Â Â Â Â Â Â Â Â Â (ANY_EXTEND:<VWIDE>
> >>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> >>
> >> The (minus should be under the "(match_operand":
> >>
> >> (define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> >> Â Â [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> >> Â Â Â Â (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
> >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â (ANY_EXTEND:<VWIDE>
> >> Â Â Â Â Â Â Â Â Â Â Â Â (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> >>
> >> Same for the other patterns.
> >>
> >> Thanks,
> >> Richard
> >>
> >
> > You will need a maintainer's approval but this looks good to me.
> > Thanks for doing this. I would only point out one other nit which you
> > can choose to ignore:
> >
> > +/* Ensure
> > +Â Â saddw2 and one saddw for the function add()
> > +Â Â ssubw2 and one ssubw for the function subtract()
> > +Â Â uaddw2 and one uaddw for the function uadd()
> > +Â Â usubw2 and one usubw for the function usubtract() */
> > +
> > +/* { dg-final { scan-assembler-times "\[ \t\]ssubw2\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]ssubw\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]saddw2\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]saddw\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]usubw2\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]usubw\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]uaddw2\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]uaddw\[ \t\]+" 1 } } */
> >
> > The scan-assembly directives for the different
> > functions can be placed right below each of them and that would
> > make it easier to read the expected results in the test and you
> > can get rid of the comments saying the same.
Thanks for the first-line review Sudi.
OK for trunk.
Thanks,
James
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns
2018-07-24 15:12 ` James Greenhalgh
@ 2018-07-24 15:39 ` Kyrill Tkachov
2018-07-30 13:30 ` Christophe Lyon
0 siblings, 1 reply; 8+ messages in thread
From: Kyrill Tkachov @ 2018-07-24 15:39 UTC (permalink / raw)
To: James Greenhalgh, Matthew Malcomson
Cc: Sudakshina Das, gcc-patches, Richard Earnshaw, Marcus Shawcroft,
Richard Sandiford, nd
On 24/07/18 16:12, James Greenhalgh wrote:
> On Thu, Jul 19, 2018 at 07:35:22AM -0500, Matthew Malcomson wrote:
> > Hi again.
> >
> > Providing an updated patch to include the formatting suggestions.
>
> Please try not to top-post replies, it makes the conversation thread
> harder to follow (reply continues below!).
>
> > On 12/07/18 11:39, Sudakshina Das wrote:
> > > Hi Matthew
> > >
> > > On 12/07/18 11:18, Richard Sandiford wrote:
> > >> Looks good to me FWIW (not a maintainer), just a minor formatting thing:
> > >>
> > >> Matthew Malcomson <matthew.malcomson@arm.com> writes:
> > >>> diff --git a/gcc/config/aarch64/aarch64-simd.md
> > >>> b/gcc/config/aarch64/aarch64-simd.md
> > >>> index
> > >>> aac5fa146ed8dde4507a0eb4ad6a07ce78d2f0cd..67b29cbe2cad91e031ee23be656ec61a403f2cf9
> > >>> 100644
> > >>> --- a/gcc/config/aarch64/aarch64-simd.md
> > >>> +++ b/gcc/config/aarch64/aarch64-simd.md
> > >>> @@ -3302,38 +3302,78 @@
> > >>> DONE;
> > >>> })
> > >>> -(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
> > >>> +(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> > >>> [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> > >>> - (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand"
> > >>> "w")
> > >>> - (ANY_EXTEND:<VWIDE>
> > >>> - (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> > >>> + (minus:<VWIDE>
> > >>> + (match_operand:<VWIDE> 1 "register_operand" "w")
> > >>> + (ANY_EXTEND:<VWIDE>
> > >>> + (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> > >>
> > >> The (minus should be under the "(match_operand":
> > >>
> > >> (define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> > >> [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> > >> (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
> > >> (ANY_EXTEND:<VWIDE>
> > >> (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> > >>
> > >> Same for the other patterns.
> > >>
> > >> Thanks,
> > >> Richard
> > >>
> > >
> > > You will need a maintainer's approval but this looks good to me.
> > > Thanks for doing this. I would only point out one other nit which you
> > > can choose to ignore:
> > >
> > > +/* Ensure
> > > + saddw2 and one saddw for the function add()
> > > + ssubw2 and one ssubw for the function subtract()
> > > + uaddw2 and one uaddw for the function uadd()
> > > + usubw2 and one usubw for the function usubtract() */
> > > +
> > > +/* { dg-final { scan-assembler-times "\[ \t\]ssubw2\[ \t\]+" 1 } } */
> > > +/* { dg-final { scan-assembler-times "\[ \t\]ssubw\[ \t\]+" 1 } } */
> > > +/* { dg-final { scan-assembler-times "\[ \t\]saddw2\[ \t\]+" 1 } } */
> > > +/* { dg-final { scan-assembler-times "\[ \t\]saddw\[ \t\]+" 1 } } */
> > > +/* { dg-final { scan-assembler-times "\[ \t\]usubw2\[ \t\]+" 1 } } */
> > > +/* { dg-final { scan-assembler-times "\[ \t\]usubw\[ \t\]+" 1 } } */
> > > +/* { dg-final { scan-assembler-times "\[ \t\]uaddw2\[ \t\]+" 1 } } */
> > > +/* { dg-final { scan-assembler-times "\[ \t\]uaddw\[ \t\]+" 1 } } */
> > >
> > > The scan-assembly directives for the different
> > > functions can be placed right below each of them and that would
> > > make it easier to read the expected results in the test and you
> > > can get rid of the comments saying the same.
>
> Thanks for the first-line review Sudi.
>
> OK for trunk.
>
I've committed this on behalf of Matthew as: https://gcc.gnu.org/r262949
Thanks,
Kyrill
> Thanks,
> James
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns
2018-07-24 15:39 ` Kyrill Tkachov
@ 2018-07-30 13:30 ` Christophe Lyon
2018-07-30 13:58 ` Kyrill Tkachov
0 siblings, 1 reply; 8+ messages in thread
From: Christophe Lyon @ 2018-07-30 13:30 UTC (permalink / raw)
To: Kyrill Tkachov
Cc: James Greenhalgh, Matthew.Malcomson, Sudi Das, gcc Patches,
Richard Earnshaw, Marcus Shawcroft, Richard Sandiford, nd
Hi,
On Tue, 24 Jul 2018 at 17:39, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
>
> On 24/07/18 16:12, James Greenhalgh wrote:
> > On Thu, Jul 19, 2018 at 07:35:22AM -0500, Matthew Malcomson wrote:
> > > Hi again.
> > >
> > > Providing an updated patch to include the formatting suggestions.
> >
> > Please try not to top-post replies, it makes the conversation thread
> > harder to follow (reply continues below!).
> >
> > > On 12/07/18 11:39, Sudakshina Das wrote:
> > > > Hi Matthew
> > > >
> > > > On 12/07/18 11:18, Richard Sandiford wrote:
> > > >> Looks good to me FWIW (not a maintainer), just a minor formatting thing:
> > > >>
> > > >> Matthew Malcomson <matthew.malcomson@arm.com> writes:
> > > >>> diff --git a/gcc/config/aarch64/aarch64-simd.md
> > > >>> b/gcc/config/aarch64/aarch64-simd.md
> > > >>> index
> > > >>> aac5fa146ed8dde4507a0eb4ad6a07ce78d2f0cd..67b29cbe2cad91e031ee23be656ec61a403f2cf9
> > > >>> 100644
> > > >>> --- a/gcc/config/aarch64/aarch64-simd.md
> > > >>> +++ b/gcc/config/aarch64/aarch64-simd.md
> > > >>> @@ -3302,38 +3302,78 @@
> > > >>> DONE;
> > > >>> })
> > > >>> -(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
> > > >>> +(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> > > >>> [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> > > >>> - (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand"
> > > >>> "w")
> > > >>> - (ANY_EXTEND:<VWIDE>
> > > >>> - (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> > > >>> + (minus:<VWIDE>
> > > >>> + (match_operand:<VWIDE> 1 "register_operand" "w")
> > > >>> + (ANY_EXTEND:<VWIDE>
> > > >>> + (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> > > >>
> > > >> The (minus should be under the "(match_operand":
> > > >>
> > > >> (define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> > > >> [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> > > >> (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
> > > >> (ANY_EXTEND:<VWIDE>
> > > >> (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> > > >>
> > > >> Same for the other patterns.
> > > >>
> > > >> Thanks,
> > > >> Richard
> > > >>
> > > >
> > > > You will need a maintainer's approval but this looks good to me.
> > > > Thanks for doing this. I would only point out one other nit which you
> > > > can choose to ignore:
> > > >
> > > > +/* Ensure
> > > > + saddw2 and one saddw for the function add()
> > > > + ssubw2 and one ssubw for the function subtract()
> > > > + uaddw2 and one uaddw for the function uadd()
> > > > + usubw2 and one usubw for the function usubtract() */
> > > > +
> > > > +/* { dg-final { scan-assembler-times "\[ \t\]ssubw2\[ \t\]+" 1 } } */
> > > > +/* { dg-final { scan-assembler-times "\[ \t\]ssubw\[ \t\]+" 1 } } */
> > > > +/* { dg-final { scan-assembler-times "\[ \t\]saddw2\[ \t\]+" 1 } } */
> > > > +/* { dg-final { scan-assembler-times "\[ \t\]saddw\[ \t\]+" 1 } } */
> > > > +/* { dg-final { scan-assembler-times "\[ \t\]usubw2\[ \t\]+" 1 } } */
> > > > +/* { dg-final { scan-assembler-times "\[ \t\]usubw\[ \t\]+" 1 } } */
> > > > +/* { dg-final { scan-assembler-times "\[ \t\]uaddw2\[ \t\]+" 1 } } */
> > > > +/* { dg-final { scan-assembler-times "\[ \t\]uaddw\[ \t\]+" 1 } } */
> > > >
> > > > The scan-assembly directives for the different
> > > > functions can be placed right below each of them and that would
> > > > make it easier to read the expected results in the test and you
> > > > can get rid of the comments saying the same.
> >
> > Thanks for the first-line review Sudi.
> >
> > OK for trunk.
> >
>
> I've committed this on behalf of Matthew as: https://gcc.gnu.org/r262949
>
The new test fail with -mabi=ilp32:
gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
\t]saddw2[ \t]+ 1
gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
\t]saddw[ \t]+ 1
gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
\t]ssubw2[ \t]+ 1
gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
\t]ssubw[ \t]+ 1
gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
\t]uaddw2[ \t]+ 1
gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
\t]uaddw[ \t]+ 1
gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
\t]usubw2[ \t]+ 1
gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
\t]usubw[ \t]+ 1
I'm not sure how much we care about ilp32?
Christophe
> Thanks,
> Kyrill
>
> > Thanks,
> > James
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns
2018-07-30 13:30 ` Christophe Lyon
@ 2018-07-30 13:58 ` Kyrill Tkachov
0 siblings, 0 replies; 8+ messages in thread
From: Kyrill Tkachov @ 2018-07-30 13:58 UTC (permalink / raw)
To: Christophe Lyon
Cc: James Greenhalgh, Matthew.Malcomson, Sudi Das, gcc Patches,
Richard Earnshaw, Marcus Shawcroft, Richard Sandiford, nd
On 30/07/18 14:30, Christophe Lyon wrote:
> Hi,
>
> On Tue, 24 Jul 2018 at 17:39, Kyrill Tkachov
> <kyrylo.tkachov@foss.arm.com> wrote:
>>
>> On 24/07/18 16:12, James Greenhalgh wrote:
>>> On Thu, Jul 19, 2018 at 07:35:22AM -0500, Matthew Malcomson wrote:
>>>> Hi again.
>>>>
>>>> Providing an updated patch to include the formatting suggestions.
>>> Please try not to top-post replies, it makes the conversation thread
>>> harder to follow (reply continues below!).
>>>
>>>> On 12/07/18 11:39, Sudakshina Das wrote:
>>>>> Hi Matthew
>>>>>
>>>>> On 12/07/18 11:18, Richard Sandiford wrote:
>>>>>> Looks good to me FWIW (not a maintainer), just a minor formatting thing:
>>>>>>
>>>>>> Matthew Malcomson <matthew.malcomson@arm.com> writes:
>>>>>>> diff --git a/gcc/config/aarch64/aarch64-simd.md
>>>>>>> b/gcc/config/aarch64/aarch64-simd.md
>>>>>>> index
>>>>>>> aac5fa146ed8dde4507a0eb4ad6a07ce78d2f0cd..67b29cbe2cad91e031ee23be656ec61a403f2cf9
>>>>>>> 100644
>>>>>>> --- a/gcc/config/aarch64/aarch64-simd.md
>>>>>>> +++ b/gcc/config/aarch64/aarch64-simd.md
>>>>>>> @@ -3302,38 +3302,78 @@
>>>>>>> DONE;
>>>>>>> })
>>>>>>> -(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
>>>>>>> +(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
>>>>>>> [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
>>>>>>> - (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand"
>>>>>>> "w")
>>>>>>> - (ANY_EXTEND:<VWIDE>
>>>>>>> - (match_operand:VD_BHSI 2 "register_operand" "w"))))]
>>>>>>> + (minus:<VWIDE>
>>>>>>> + (match_operand:<VWIDE> 1 "register_operand" "w")
>>>>>>> + (ANY_EXTEND:<VWIDE>
>>>>>>> + (match_operand:VD_BHSI 2 "register_operand" "w"))))]
>>>>>> The (minus should be under the "(match_operand":
>>>>>>
>>>>>> (define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
>>>>>> [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
>>>>>> (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
>>>>>> (ANY_EXTEND:<VWIDE>
>>>>>> (match_operand:VD_BHSI 2 "register_operand" "w"))))]
>>>>>>
>>>>>> Same for the other patterns.
>>>>>>
>>>>>> Thanks,
>>>>>> Richard
>>>>>>
>>>>> You will need a maintainer's approval but this looks good to me.
>>>>> Thanks for doing this. I would only point out one other nit which you
>>>>> can choose to ignore:
>>>>>
>>>>> +/* Ensure
>>>>> + saddw2 and one saddw for the function add()
>>>>> + ssubw2 and one ssubw for the function subtract()
>>>>> + uaddw2 and one uaddw for the function uadd()
>>>>> + usubw2 and one usubw for the function usubtract() */
>>>>> +
>>>>> +/* { dg-final { scan-assembler-times "\[ \t\]ssubw2\[ \t\]+" 1 } } */
>>>>> +/* { dg-final { scan-assembler-times "\[ \t\]ssubw\[ \t\]+" 1 } } */
>>>>> +/* { dg-final { scan-assembler-times "\[ \t\]saddw2\[ \t\]+" 1 } } */
>>>>> +/* { dg-final { scan-assembler-times "\[ \t\]saddw\[ \t\]+" 1 } } */
>>>>> +/* { dg-final { scan-assembler-times "\[ \t\]usubw2\[ \t\]+" 1 } } */
>>>>> +/* { dg-final { scan-assembler-times "\[ \t\]usubw\[ \t\]+" 1 } } */
>>>>> +/* { dg-final { scan-assembler-times "\[ \t\]uaddw2\[ \t\]+" 1 } } */
>>>>> +/* { dg-final { scan-assembler-times "\[ \t\]uaddw\[ \t\]+" 1 } } */
>>>>>
>>>>> The scan-assembly directives for the different
>>>>> functions can be placed right below each of them and that would
>>>>> make it easier to read the expected results in the test and you
>>>>> can get rid of the comments saying the same.
>>> Thanks for the first-line review Sudi.
>>>
>>> OK for trunk.
>>>
>> I've committed this on behalf of Matthew as: https://gcc.gnu.org/r262949
>>
> The new test fail with -mabi=ilp32:
> gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
> \t]saddw2[ \t]+ 1
> gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
> \t]saddw[ \t]+ 1
> gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
> \t]ssubw2[ \t]+ 1
> gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
> \t]ssubw[ \t]+ 1
> gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
> \t]uaddw2[ \t]+ 1
> gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
> \t]uaddw[ \t]+ 1
> gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
> \t]usubw2[ \t]+ 1
> gcc.target/aarch64/simd/vect_su_add_sub.c scan-assembler-times [
> \t]usubw[ \t]+ 1
>
> I'm not sure how much we care about ilp32?
Looks like the usual pitfall of unsigned longs being 32-bit in ILP32.
The test should be made to use "long long"s that are guaranteed to be 64 bits
instead of just longs
Thanks,
Kyrill
> Christophe
>
>> Thanks,
>> Kyrill
>>
>>> Thanks,
>>> James
>>>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-30 13:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-11 10:36 [PATCH][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns Matthew Malcomson
2018-07-12 10:18 ` Richard Sandiford
2018-07-12 10:39 ` Sudakshina Das
2018-07-19 12:35 ` Matthew Malcomson
2018-07-24 15:12 ` James Greenhalgh
2018-07-24 15:39 ` Kyrill Tkachov
2018-07-30 13:30 ` Christophe Lyon
2018-07-30 13:58 ` Kyrill Tkachov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).