Re: [PATCH v2] AArch64: Fix memmove operand corruption [PR111121]

[Richard Sandiford <richard.sandiford@arm.com>]

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