* [PATCH v2] AArch64: Fix memmove operand corruption [PR111121]
@ 2023-09-20 16:07 Wilco Dijkstra
2023-09-28 10:47 ` Richard Sandiford
0 siblings, 1 reply; 2+ messages in thread
From: Wilco Dijkstra @ 2023-09-20 16:07 UTC (permalink / raw)
To: GCC Patches; +Cc: Richard Sandiford
A MOPS memmove may corrupt registers since there is no copy of the input
operands to temporary registers. Fix this by calling
aarch64_expand_cpymem_mops.
Passes regress/bootstrap, OK for commit?
gcc/ChangeLog/
PR target/111121
* config/aarch64/aarch64.md (aarch64_movmemdi): Add new expander.
(movmemdi): Call aarch64_expand_cpymem_mops for correct expansion.
* config/aarch64/aarch64.cc (aarch64_expand_cpymem_mops): Add support
for memmove.
* config/aarch64/aarch64-protos.h (aarch64_expand_cpymem_mops): Add new
function.
gcc/testsuite/ChangeLog/
PR target/111121
* gcc.target/aarch64/mops_4.c: Add memmove testcases.
---
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 70303d6fd953e0c397b9138ede8858c2db2e53db..e8d91cba30e32e03c4794ccc24254691d135f2dd 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -765,6 +765,7 @@ bool aarch64_emit_approx_div (rtx, rtx, rtx);
bool aarch64_emit_approx_sqrt (rtx, rtx, bool);
tree aarch64_vector_load_decl (tree);
void aarch64_expand_call (rtx, rtx, rtx, bool);
+bool aarch64_expand_cpymem_mops (rtx *, bool);
bool aarch64_expand_cpymem (rtx *);
bool aarch64_expand_setmem (rtx *);
bool aarch64_float_const_zero_rtx_p (rtx);
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 219c4ee6d4cd7522f6ad634c794485841e5d08fa..dd6874d13a75f20d10a244578afc355b25c73da2 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -25228,10 +25228,11 @@ aarch64_copy_one_block_and_progress_pointers (rtx *src, rtx *dst,
*dst = aarch64_progress_pointer (*dst);
}
-/* Expand a cpymem using the MOPS extension. OPERANDS are taken
- from the cpymem pattern. Return true iff we succeeded. */
-static bool
-aarch64_expand_cpymem_mops (rtx *operands)
+/* Expand a cpymem/movmem using the MOPS extension. OPERANDS are taken
+ from the cpymem/movmem pattern. IS_MEMMOVE is true if this is a memmove
+ rather than memcpy. Return true iff we succeeded. */
+bool
+aarch64_expand_cpymem_mops (rtx *operands, bool is_memmove = false)
{
if (!TARGET_MOPS)
return false;
@@ -25243,8 +25244,10 @@ aarch64_expand_cpymem_mops (rtx *operands)
rtx dst_mem = replace_equiv_address (operands[0], dst_addr);
rtx src_mem = replace_equiv_address (operands[1], src_addr);
rtx sz_reg = copy_to_mode_reg (DImode, operands[2]);
- emit_insn (gen_aarch64_cpymemdi (dst_mem, src_mem, sz_reg));
-
+ if (is_memmove)
+ emit_insn (gen_aarch64_movmemdi (dst_mem, src_mem, sz_reg));
+ else
+ emit_insn (gen_aarch64_cpymemdi (dst_mem, src_mem, sz_reg));
return true;
}
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 60133b541e9289610ce58116b0258a61f29bdc00..6d0f072a9dd6d094e8764a513222a9129d8296fa 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1635,7 +1635,22 @@ (define_expand "cpymemdi"
}
)
-(define_insn "aarch64_movmemdi"
+(define_expand "aarch64_movmemdi"
+ [(parallel
+ [(set (match_operand 2) (const_int 0))
+ (clobber (match_dup 3))
+ (clobber (match_dup 4))
+ (clobber (reg:CC CC_REGNUM))
+ (set (match_operand 0)
+ (unspec:BLK [(match_operand 1) (match_dup 2)] UNSPEC_MOVMEM))])]
+ "TARGET_MOPS"
+ {
+ operands[3] = XEXP (operands[0], 0);
+ operands[4] = XEXP (operands[1], 0);
+ }
+)
+
+(define_insn "*aarch64_movmemdi"
[(parallel [
(set (match_operand:DI 2 "register_operand" "+&r") (const_int 0))
(clobber (match_operand:DI 0 "register_operand" "+&r"))
@@ -1668,17 +1683,9 @@ (define_expand "movmemdi"
&& INTVAL (sz_reg) < aarch64_mops_memmove_size_threshold)
FAIL;
- rtx addr_dst = XEXP (operands[0], 0);
- rtx addr_src = XEXP (operands[1], 0);
-
- if (!REG_P (sz_reg))
- sz_reg = force_reg (DImode, sz_reg);
- if (!REG_P (addr_dst))
- addr_dst = force_reg (DImode, addr_dst);
- if (!REG_P (addr_src))
- addr_src = force_reg (DImode, addr_src);
- emit_insn (gen_aarch64_movmemdi (addr_dst, addr_src, sz_reg));
- DONE;
+ if (aarch64_expand_cpymem_mops (operands, true))
+ DONE;
+ FAIL;
}
)
diff --git a/gcc/testsuite/gcc.target/aarch64/mops_4.c b/gcc/testsuite/gcc.target/aarch64/mops_4.c
index 1b87759cb5e8bbcbb58cf63404d1d579d44b2818..dd796115cb4093251964d881e93bf4b98ade0c32 100644
--- a/gcc/testsuite/gcc.target/aarch64/mops_4.c
+++ b/gcc/testsuite/gcc.target/aarch64/mops_4.c
@@ -50,6 +50,54 @@ copy3 (int *x, int *y, long z, long *res)
*res = z;
}
+/*
+** move1:
+** mov (x[0-9]+), x0
+** cpyp \[\1\]!, \[x1\]!, x2!
+** cpym \[\1\]!, \[x1\]!, x2!
+** cpye \[\1\]!, \[x1\]!, x2!
+** str x0, \[x3\]
+** ret
+*/
+void
+move1 (int *x, int *y, long z, int **res)
+{
+ __builtin_memmove (x, y, z);
+ *res = x;
+}
+
+/*
+** move2:
+** mov (x[0-9]+), x1
+** cpyp \[x0\]!, \[\1\]!, x2!
+** cpym \[x0\]!, \[\1\]!, x2!
+** cpye \[x0\]!, \[\1\]!, x2!
+** str x1, \[x3\]
+** ret
+*/
+void
+move2 (int *x, int *y, long z, int **res)
+{
+ __builtin_memmove (x, y, z);
+ *res = y;
+}
+
+/*
+** move3:
+** mov (x[0-9]+), x2
+** cpyp \[x0\]!, \[x1\]!, \1!
+** cpym \[x0\]!, \[x1\]!, \1!
+** cpye \[x0\]!, \[x1\]!, \1!
+** str x2, \[x3\]
+** ret
+*/
+void
+move3 (int *x, int *y, long z, long *res)
+{
+ __builtin_memmove (x, y, z);
+ *res = z;
+}
+
/*
** set1:
** mov (x[0-9]+), x0
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2] AArch64: Fix memmove operand corruption [PR111121]
2023-09-20 16:07 [PATCH v2] AArch64: Fix memmove operand corruption [PR111121] Wilco Dijkstra
@ 2023-09-28 10:47 ` Richard Sandiford
0 siblings, 0 replies; 2+ messages in thread
From: Richard Sandiford @ 2023-09-28 10:47 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: GCC Patches
Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> A MOPS memmove may corrupt registers since there is no copy of the input
> operands to temporary registers. Fix this by calling
> aarch64_expand_cpymem_mops.
>
> Passes regress/bootstrap, OK for commit?
>
> gcc/ChangeLog/
> PR target/111121
> * config/aarch64/aarch64.md (aarch64_movmemdi): Add new expander.
> (movmemdi): Call aarch64_expand_cpymem_mops for correct expansion.
> * config/aarch64/aarch64.cc (aarch64_expand_cpymem_mops): Add support
> for memmove.
> * config/aarch64/aarch64-protos.h (aarch64_expand_cpymem_mops): Add new
> function.
>
> gcc/testsuite/ChangeLog/
> PR target/111121
> * gcc.target/aarch64/mops_4.c: Add memmove testcases.
OK, thanks. Also OK for whichever branches need it.
Sorry for the slow review, too much email backlog :(
Richard
>
> ---
>
> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> index 70303d6fd953e0c397b9138ede8858c2db2e53db..e8d91cba30e32e03c4794ccc24254691d135f2dd 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -765,6 +765,7 @@ bool aarch64_emit_approx_div (rtx, rtx, rtx);
> bool aarch64_emit_approx_sqrt (rtx, rtx, bool);
> tree aarch64_vector_load_decl (tree);
> void aarch64_expand_call (rtx, rtx, rtx, bool);
> +bool aarch64_expand_cpymem_mops (rtx *, bool);
> bool aarch64_expand_cpymem (rtx *);
> bool aarch64_expand_setmem (rtx *);
> bool aarch64_float_const_zero_rtx_p (rtx);
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 219c4ee6d4cd7522f6ad634c794485841e5d08fa..dd6874d13a75f20d10a244578afc355b25c73da2 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -25228,10 +25228,11 @@ aarch64_copy_one_block_and_progress_pointers (rtx *src, rtx *dst,
> *dst = aarch64_progress_pointer (*dst);
> }
>
> -/* Expand a cpymem using the MOPS extension. OPERANDS are taken
> - from the cpymem pattern. Return true iff we succeeded. */
> -static bool
> -aarch64_expand_cpymem_mops (rtx *operands)
> +/* Expand a cpymem/movmem using the MOPS extension. OPERANDS are taken
> + from the cpymem/movmem pattern. IS_MEMMOVE is true if this is a memmove
> + rather than memcpy. Return true iff we succeeded. */
> +bool
> +aarch64_expand_cpymem_mops (rtx *operands, bool is_memmove = false)
> {
> if (!TARGET_MOPS)
> return false;
> @@ -25243,8 +25244,10 @@ aarch64_expand_cpymem_mops (rtx *operands)
> rtx dst_mem = replace_equiv_address (operands[0], dst_addr);
> rtx src_mem = replace_equiv_address (operands[1], src_addr);
> rtx sz_reg = copy_to_mode_reg (DImode, operands[2]);
> - emit_insn (gen_aarch64_cpymemdi (dst_mem, src_mem, sz_reg));
> -
> + if (is_memmove)
> + emit_insn (gen_aarch64_movmemdi (dst_mem, src_mem, sz_reg));
> + else
> + emit_insn (gen_aarch64_cpymemdi (dst_mem, src_mem, sz_reg));
> return true;
> }
>
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 60133b541e9289610ce58116b0258a61f29bdc00..6d0f072a9dd6d094e8764a513222a9129d8296fa 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -1635,7 +1635,22 @@ (define_expand "cpymemdi"
> }
> )
>
> -(define_insn "aarch64_movmemdi"
> +(define_expand "aarch64_movmemdi"
> + [(parallel
> + [(set (match_operand 2) (const_int 0))
> + (clobber (match_dup 3))
> + (clobber (match_dup 4))
> + (clobber (reg:CC CC_REGNUM))
> + (set (match_operand 0)
> + (unspec:BLK [(match_operand 1) (match_dup 2)] UNSPEC_MOVMEM))])]
> + "TARGET_MOPS"
> + {
> + operands[3] = XEXP (operands[0], 0);
> + operands[4] = XEXP (operands[1], 0);
> + }
> +)
> +
> +(define_insn "*aarch64_movmemdi"
> [(parallel [
> (set (match_operand:DI 2 "register_operand" "+&r") (const_int 0))
> (clobber (match_operand:DI 0 "register_operand" "+&r"))
> @@ -1668,17 +1683,9 @@ (define_expand "movmemdi"
> && INTVAL (sz_reg) < aarch64_mops_memmove_size_threshold)
> FAIL;
>
> - rtx addr_dst = XEXP (operands[0], 0);
> - rtx addr_src = XEXP (operands[1], 0);
> -
> - if (!REG_P (sz_reg))
> - sz_reg = force_reg (DImode, sz_reg);
> - if (!REG_P (addr_dst))
> - addr_dst = force_reg (DImode, addr_dst);
> - if (!REG_P (addr_src))
> - addr_src = force_reg (DImode, addr_src);
> - emit_insn (gen_aarch64_movmemdi (addr_dst, addr_src, sz_reg));
> - DONE;
> + if (aarch64_expand_cpymem_mops (operands, true))
> + DONE;
> + FAIL;
> }
> )
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/mops_4.c b/gcc/testsuite/gcc.target/aarch64/mops_4.c
> index 1b87759cb5e8bbcbb58cf63404d1d579d44b2818..dd796115cb4093251964d881e93bf4b98ade0c32 100644
> --- a/gcc/testsuite/gcc.target/aarch64/mops_4.c
> +++ b/gcc/testsuite/gcc.target/aarch64/mops_4.c
> @@ -50,6 +50,54 @@ copy3 (int *x, int *y, long z, long *res)
> *res = z;
> }
>
> +/*
> +** move1:
> +** mov (x[0-9]+), x0
> +** cpyp \[\1\]!, \[x1\]!, x2!
> +** cpym \[\1\]!, \[x1\]!, x2!
> +** cpye \[\1\]!, \[x1\]!, x2!
> +** str x0, \[x3\]
> +** ret
> +*/
> +void
> +move1 (int *x, int *y, long z, int **res)
> +{
> + __builtin_memmove (x, y, z);
> + *res = x;
> +}
> +
> +/*
> +** move2:
> +** mov (x[0-9]+), x1
> +** cpyp \[x0\]!, \[\1\]!, x2!
> +** cpym \[x0\]!, \[\1\]!, x2!
> +** cpye \[x0\]!, \[\1\]!, x2!
> +** str x1, \[x3\]
> +** ret
> +*/
> +void
> +move2 (int *x, int *y, long z, int **res)
> +{
> + __builtin_memmove (x, y, z);
> + *res = y;
> +}
> +
> +/*
> +** move3:
> +** mov (x[0-9]+), x2
> +** cpyp \[x0\]!, \[x1\]!, \1!
> +** cpym \[x0\]!, \[x1\]!, \1!
> +** cpye \[x0\]!, \[x1\]!, \1!
> +** str x2, \[x3\]
> +** ret
> +*/
> +void
> +move3 (int *x, int *y, long z, long *res)
> +{
> + __builtin_memmove (x, y, z);
> + *res = z;
> +}
> +
> /*
> ** set1:
> ** mov (x[0-9]+), x0
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-09-28 10:47 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-20 16:07 [PATCH v2] AArch64: Fix memmove operand corruption [PR111121] Wilco Dijkstra
2023-09-28 10:47 ` Richard Sandiford
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).