From: Jackson Woodruff <jackson.woodruff@foss.arm.com>
To: "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>,
gcc-patches@gcc.gnu.org
Cc: James Greenhalgh <james.greenhalgh@arm.com>
Subject: Re: [AArch64, PATCH] Improve Neon store of zero
Date: Wed, 16 Aug 2017 16:01:00 -0000 [thread overview]
Message-ID: <f7e9a7ec-b2d5-dcdd-d67b-e6f246385738@foss.arm.com> (raw)
In-Reply-To: <02cb536e-0214-ae4f-df22-d18a82087990@arm.com>
[-- Attachment #1: Type: text/plain, Size: 6194 bytes --]
Hi Richard,
I have changed the condition as you suggest below. OK for trunk?
Jackson.
On 08/11/2017 02:56 PM, Richard Earnshaw (lists) wrote:
> On 10/08/17 14:12, Jackson Woodruff wrote:
>> Hi all,
>>
>> This patch changes patterns in aarch64-simd.md to replace
>>
>> movi v0.4s, 0
>> str q0, [x0, 16]
>>
>> With:
>>
>> stp xzr, xzr, [x0, 16]
>>
>> When we are storing zeros to vectors like this:
>>
>> void f(uint32x4_t *p) {
>> uint32x4_t x = { 0, 0, 0, 0};
>> p[1] = x;
>> }
>>
>> Bootstrapped and regtested on aarch64 with no regressions.
>> OK for trunk?
>>
>> Jackson
>>
>> gcc/
>>
>> 2017-08-09 Jackson Woodruff <jackson.woodruff@arm.com>
>>
>> * aarch64-simd.md (mov<mode>): No longer force zero
>> immediate into register.
>> (*aarch64_simd_mov<mode>): Add new case for stp
>> using zero immediate.
>>
>>
>> gcc/testsuite
>>
>> 2017-08-09 Jackson Woodruff <jackson.woodruff@arm.com>
>>
>> * gcc.target/aarch64/simd/neon_str_zero.c: New.
>>
>>
>> patchfile
>>
>>
>> diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
>> index 74de9b8c89dd5e4e3d87504594c969de0e0128ce..0149a742d34ae4fd5b3fd705b03c845f94aa1d59 100644
>> --- a/gcc/config/aarch64/aarch64-simd.md
>> +++ b/gcc/config/aarch64/aarch64-simd.md
>> @@ -23,7 +23,10 @@
>> (match_operand:VALL_F16 1 "general_operand" ""))]
>> "TARGET_SIMD"
>> "
>> - if (GET_CODE (operands[0]) == MEM)
>> + if (GET_CODE (operands[0]) == MEM
>> + && !(aarch64_simd_imm_zero (operands[1], <MODE>mode)
>> + && aarch64_legitimate_address_p (<MODE>mode, operands[0],
>> + PARALLEL, 1)))
>> operands[1] = force_reg (<MODE>mode, operands[1]);
>> "
>> )
>> @@ -94,63 +97,70 @@
>>
>> (define_insn "*aarch64_simd_mov<mode>"
>> [(set (match_operand:VD 0 "nonimmediate_operand"
>> - "=w, m, w, ?r, ?w, ?r, w")
>> + "=w, m, m, w, ?r, ?w, ?r, w")
>> (match_operand:VD 1 "general_operand"
>> - "m, w, w, w, r, r, Dn"))]
>> + "m, Dz, w, w, w, r, r, Dn"))]
>> "TARGET_SIMD
>> - && (register_operand (operands[0], <MODE>mode)
>> - || register_operand (operands[1], <MODE>mode))"
>> + && ((register_operand (operands[0], <MODE>mode)
>> + || register_operand (operands[1], <MODE>mode))
>> + || (memory_operand (operands[0], <MODE>mode)
>> + && immediate_operand (operands[1], <MODE>mode)))"
> Allowing any immediate here seems too lax - it allows any immediate
> value which then could cause reload operations to be inserted (that in
> turn might cause register pressure calculations to be incorrect).
> Wouldn't it be better to use something like aarch64_simd_reg_or_zero?
> Similarly below.
>
> R.
>
>> {
>> switch (which_alternative)
>> {
>> case 0: return "ldr\\t%d0, %1";
>> - case 1: return "str\\t%d1, %0";
>> - case 2: return "mov\t%0.<Vbtype>, %1.<Vbtype>";
>> - case 3: return "umov\t%0, %1.d[0]";
>> - case 4: return "fmov\t%d0, %1";
>> - case 5: return "mov\t%0, %1";
>> - case 6:
>> + case 1: return "str\\txzr, %0";
>> + case 2: return "str\\t%d1, %0";
>> + case 3: return "mov\t%0.<Vbtype>, %1.<Vbtype>";
>> + case 4: return "umov\t%0, %1.d[0]";
>> + case 5: return "fmov\t%d0, %1";
>> + case 6: return "mov\t%0, %1";
>> + case 7:
>> return aarch64_output_simd_mov_immediate (operands[1],
>> <MODE>mode, 64);
>> default: gcc_unreachable ();
>> }
>> }
>> - [(set_attr "type" "neon_load1_1reg<q>, neon_store1_1reg<q>,\
>> + [(set_attr "type" "neon_load1_1reg<q>, neon_stp, neon_store1_1reg<q>,\
>> neon_logic<q>, neon_to_gp<q>, f_mcr,\
>> mov_reg, neon_move<q>")]
>> )
>>
>> (define_insn "*aarch64_simd_mov<mode>"
>> [(set (match_operand:VQ 0 "nonimmediate_operand"
>> - "=w, m, w, ?r, ?w, ?r, w")
>> + "=w, Ump, m, w, ?r, ?w, ?r, w")
>> (match_operand:VQ 1 "general_operand"
>> - "m, w, w, w, r, r, Dn"))]
>> + "m, Dz, w, w, w, r, r, Dn"))]
>> "TARGET_SIMD
>> - && (register_operand (operands[0], <MODE>mode)
>> - || register_operand (operands[1], <MODE>mode))"
>> + && ((register_operand (operands[0], <MODE>mode)
>> + || register_operand (operands[1], <MODE>mode))
>> + || (memory_operand (operands[0], <MODE>mode)
>> + && immediate_operand (operands[1], <MODE>mode)))"
>> {
>> switch (which_alternative)
>> {
>> case 0:
>> return "ldr\\t%q0, %1";
>> case 1:
>> - return "str\\t%q1, %0";
>> + return "stp\\txzr, xzr, %0";
>> case 2:
>> - return "mov\t%0.<Vbtype>, %1.<Vbtype>";
>> + return "str\\t%q1, %0";
>> case 3:
>> + return "mov\t%0.<Vbtype>, %1.<Vbtype>";
>> case 4:
>> case 5:
>> - return "#";
>> case 6:
>> + return "#";
>> + case 7:
>> return aarch64_output_simd_mov_immediate (operands[1], <MODE>mode, 128);
>> default:
>> gcc_unreachable ();
>> }
>> }
>> [(set_attr "type" "neon_load1_1reg<q>, neon_store1_1reg<q>,\
>> - neon_logic<q>, multiple, multiple, multiple,\
>> - neon_move<q>")
>> - (set_attr "length" "4,4,4,8,8,8,4")]
>> + neon_stp, neon_logic<q>, multiple, multiple,\
>> + multiple, neon_move<q>")
>> + (set_attr "length" "4,4,4,4,8,8,8,4")]
>> )
>>
>> ;; When storing lane zero we can use the normal STR and its more permissive
>> diff --git a/gcc/testsuite/gcc.target/aarch64/simd/neon_str_zero.c b/gcc/testsuite/gcc.target/aarch64/simd/neon_str_zero.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..07198de109432b530745cc540790303ae0245efb
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/simd/neon_str_zero.c
>> @@ -0,0 +1,22 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O1" } */
>> +
>> +#include <arm_neon.h>
>> +
>> +void
>> +f (uint32x4_t *p)
>> +{
>> + uint32x4_t x = { 0, 0, 0, 0};
>> + p[1] = x;
>> +
>> + /* { dg-final { scan-assembler "stp\txzr, xzr," } } */
>> +}
>> +
>> +void
>> +g (float32x2_t *p)
>> +{
>> + float32x2_t x = {0.0, 0.0};
>> + p[0] = x;
>> +
>> + /* { dg-final { scan-assembler "str\txzr, " } } */
>> +}
>>
[-- Attachment #2: patchfile --]
[-- Type: text/plain, Size: 4140 bytes --]
diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index 74de9b8c89dd5e4e3d87504594c969de0e0128ce..ce1b981fc005edf48a401a456def2a37cf9d9022 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -23,7 +23,10 @@
(match_operand:VALL_F16 1 "general_operand" ""))]
"TARGET_SIMD"
"
- if (GET_CODE (operands[0]) == MEM)
+ if (GET_CODE (operands[0]) == MEM
+ && !(aarch64_simd_imm_zero (operands[1], <MODE>mode)
+ && aarch64_legitimate_address_p (<MODE>mode, operands[0],
+ PARALLEL, 1)))
operands[1] = force_reg (<MODE>mode, operands[1]);
"
)
@@ -94,63 +97,66 @@
(define_insn "*aarch64_simd_mov<mode>"
[(set (match_operand:VD 0 "nonimmediate_operand"
- "=w, m, w, ?r, ?w, ?r, w")
+ "=w, m, m, w, ?r, ?w, ?r, w")
(match_operand:VD 1 "general_operand"
- "m, w, w, w, r, r, Dn"))]
+ "m, Dz, w, w, w, r, r, Dn"))]
"TARGET_SIMD
&& (register_operand (operands[0], <MODE>mode)
- || register_operand (operands[1], <MODE>mode))"
+ || aarch64_simd_reg_or_zero (operands[1], <MODE>mode))"
{
switch (which_alternative)
{
- case 0: return "ldr\\t%d0, %1";
- case 1: return "str\\t%d1, %0";
- case 2: return "mov\t%0.<Vbtype>, %1.<Vbtype>";
- case 3: return "umov\t%0, %1.d[0]";
- case 4: return "fmov\t%d0, %1";
- case 5: return "mov\t%0, %1";
- case 6:
+ case 0: return "ldr\t%d0, %1";
+ case 1: return "str\txzr, %0";
+ case 2: return "str\t%d1, %0";
+ case 3: return "mov\t%0.<Vbtype>, %1.<Vbtype>";
+ case 4: return "umov\t%0, %1.d[0]";
+ case 5: return "fmov\t%d0, %1";
+ case 6: return "mov\t%0, %1";
+ case 7:
return aarch64_output_simd_mov_immediate (operands[1],
<MODE>mode, 64);
default: gcc_unreachable ();
}
}
- [(set_attr "type" "neon_load1_1reg<q>, neon_store1_1reg<q>,\
+ [(set_attr "type" "neon_load1_1reg<q>, neon_stp, neon_store1_1reg<q>,\
neon_logic<q>, neon_to_gp<q>, f_mcr,\
mov_reg, neon_move<q>")]
)
(define_insn "*aarch64_simd_mov<mode>"
[(set (match_operand:VQ 0 "nonimmediate_operand"
- "=w, m, w, ?r, ?w, ?r, w")
+ "=w, Ump, m, w, ?r, ?w, ?r, w")
(match_operand:VQ 1 "general_operand"
- "m, w, w, w, r, r, Dn"))]
+ "m, Dz, w, w, w, r, r, Dn"))]
"TARGET_SIMD
&& (register_operand (operands[0], <MODE>mode)
- || register_operand (operands[1], <MODE>mode))"
+ || aarch64_simd_reg_or_zero (operands[1], <MODE>mode))"
{
switch (which_alternative)
{
case 0:
- return "ldr\\t%q0, %1";
+ return "ldr\t%q0, %1";
case 1:
- return "str\\t%q1, %0";
+ return "stp\txzr, xzr, %0";
case 2:
- return "mov\t%0.<Vbtype>, %1.<Vbtype>";
+ return "str\t%q1, %0";
case 3:
+ return "mov\t%0.<Vbtype>, %1.<Vbtype>";
case 4:
case 5:
- return "#";
case 6:
+ return "#";
+ case 7:
return aarch64_output_simd_mov_immediate (operands[1], <MODE>mode, 128);
default:
gcc_unreachable ();
}
}
[(set_attr "type" "neon_load1_1reg<q>, neon_store1_1reg<q>,\
- neon_logic<q>, multiple, multiple, multiple,\
- neon_move<q>")
- (set_attr "length" "4,4,4,8,8,8,4")]
+ neon_stp, neon_logic<q>, multiple, multiple,\
+ multiple, neon_move<q>")
+ (set_attr "length" "4,4,4,4,8,8,8,4")]
)
;; When storing lane zero we can use the normal STR and its more permissive
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/neon_str_zero.c b/gcc/testsuite/gcc.target/aarch64/simd/neon_str_zero.c
new file mode 100644
index 0000000000000000000000000000000000000000..07198de109432b530745cc540790303ae0245efb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/simd/neon_str_zero.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+#include <arm_neon.h>
+
+void
+f (uint32x4_t *p)
+{
+ uint32x4_t x = { 0, 0, 0, 0};
+ p[1] = x;
+
+ /* { dg-final { scan-assembler "stp\txzr, xzr," } } */
+}
+
+void
+g (float32x2_t *p)
+{
+ float32x2_t x = {0.0, 0.0};
+ p[0] = x;
+
+ /* { dg-final { scan-assembler "str\txzr, " } } */
+}
next prev parent reply other threads:[~2017-08-16 15:19 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-10 13:38 Jackson Woodruff
2017-08-11 15:16 ` Richard Earnshaw (lists)
2017-08-16 16:01 ` Jackson Woodruff [this message]
2017-08-17 13:56 ` Richard Earnshaw (lists)
2017-08-23 14:46 ` Richard Sandiford
2017-08-23 15:06 Wilco Dijkstra
2017-09-06 9:03 ` Jackson Woodruff
2017-09-12 16:28 ` James Greenhalgh
2017-09-13 16:35 ` Jackson Woodruff
2017-09-13 16:51 ` James Greenhalgh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f7e9a7ec-b2d5-dcdd-d67b-e6f246385738@foss.arm.com \
--to=jackson.woodruff@foss.arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=james.greenhalgh@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).