arm: Fix LDRD register overlap [PR117675]
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Test passed
|
Commit Message
The register indexed variants of LDRD have complex register overlap constraints
which makes them hard to use without using output_move_double (which can't be
used for atomics as it doesn't guarantee to emit atomic LDRD/STRD when required).
Add a new predicate and constraint for plain LDRD/STRD with base or base+imm.
This blocks register indexing and fixes PR117675.
Passes regress & bootstrap, OK for commit & backport to GCC14?
gcc:
PR target/117675
* config/arm/arm.cc (arm_ldrd_legitimate_address): New function.
* config/arm/arm-protos.h (arm_ldrd_legitimate_address): New prototype.
* config/arm/constraints.md: Add new Uo constraint.
* config/arm/predicates.md (arm_ldrd_memory_operand): Add new predicate.
* config/arm/sync.md (arm_atomic_loaddi2_ldrd): Use arm_ldrd_memory_operand and Uo.
gcc/testsuite:
PR target/117675
* gcc.target/arm/pr117675.c: Add new test.
---
Comments
On 03/12/2024 16:09, Wilco Dijkstra wrote:
>
> The register indexed variants of LDRD have complex register overlap constraints
> which makes them hard to use without using output_move_double (which can't be
> used for atomics as it doesn't guarantee to emit atomic LDRD/STRD when required).
> Add a new predicate and constraint for plain LDRD/STRD with base or base+imm.
> This blocks register indexing and fixes PR117675.
>
> Passes regress & bootstrap, OK for commit & backport to GCC14?
>
> gcc:
> PR target/117675
> * config/arm/arm.cc (arm_ldrd_legitimate_address): New function.
> * config/arm/arm-protos.h (arm_ldrd_legitimate_address): New prototype.
> * config/arm/constraints.md: Add new Uo constraint.
> * config/arm/predicates.md (arm_ldrd_memory_operand): Add new predicate.
> * config/arm/sync.md (arm_atomic_loaddi2_ldrd): Use arm_ldrd_memory_operand and Uo.
>
> gcc/testsuite:
> PR target/117675
> * gcc.target/arm/pr117675.c: Add new test.
>
This is OK.
If sometimes pondered if it would be better if GCC could support
addressing mode descriptions via RTL rules, something like
(define_address "ldrd_atomic"
(parallel
[(match_operand:SI 0 "register_operand" "r")
(pre_inc:SI (match_operand:SI 0 "register_operand" "+r"))
(pre_modify:SI (match_operand 0 "register_operand" "+&r,r")
(match_operand 1 "reg_or_ldrd_offset" "r,Cn"))
...
])
This would allow us to much more flexibly describe constraints such as
early clobbers, or other register classes that can be used for addressing.
But that's for another day.
R.
> ---
>
> diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
> index 7311ad4d8e4a4fae88e3d194a553252fbef3fae9..fee20239e5887a59cac8721685bc07341eff1fa8 100644
> --- a/gcc/config/arm/arm-protos.h
> +++ b/gcc/config/arm/arm-protos.h
> @@ -202,6 +202,7 @@ extern rtx arm_load_tp (rtx);
> extern bool arm_coproc_builtin_available (enum unspecv);
> extern bool arm_coproc_ldc_stc_legitimate_address (rtx);
> extern rtx arm_stack_protect_tls_canary_mem (bool);
> +extern bool arm_ldrd_legitimate_address (rtx);
>
>
> #if defined TREE_CODE
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index 0f72f3a9031237192c6362760203fe489946b948..ef396368e75ccf63833a834c7f5725f76a1a8f1b 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -34481,6 +34481,30 @@ arm_coproc_ldc_stc_legitimate_address (rtx op)
> return false;
> }
>
> +/* Return true if OP is a valid memory operand for LDRD/STRD without any
> + register overlap restrictions. Allow [base] and [base, imm] for now. */
> +bool
> +arm_ldrd_legitimate_address (rtx op)
> +{
> + if (!MEM_P (op))
> + return false;
> +
> + op = XEXP (op, 0);
> + if (REG_P (op))
> + return true;
> +
> + if (GET_CODE (op) != PLUS)
> + return false;
> + if (!REG_P (XEXP (op, 0)) || !CONST_INT_P (XEXP (op, 1)))
> + return false;
> +
> + HOST_WIDE_INT val = INTVAL (XEXP (op, 1));
> +
> + if (TARGET_ARM)
> + return IN_RANGE (val, -255, 255);
> + return IN_RANGE (val, -1020, 1020) && (val & 3) == 0;
> +}
> +
> /* Return the diagnostic message string if conversion from FROMTYPE to
> TOTYPE is not allowed, NULL otherwise. */
>
> diff --git a/gcc/config/arm/constraints.md b/gcc/config/arm/constraints.md
> index cd15967014536cd9923f4b1564ac6e3e70fbc94b..57d8fef0bd63b1e4ade74de8ccbb524b83a6c3d1 100644
> --- a/gcc/config/arm/constraints.md
> +++ b/gcc/config/arm/constraints.md
> @@ -39,7 +39,7 @@
> ;; in all states: Pg
>
> ;; The following memory constraints have been used:
> -;; in ARM/Thumb-2 state: Uh, Ut, Uv, Uy, Un, Um, Us, Up, Uf, Ux, Ul
> +;; in ARM/Thumb-2 state: Uh, Ut, Uv, Uy, Un, Um, Us, Uo, Up, Uf, Ux, Ul, Uz
> ;; in ARM state: Uq
> ;; in Thumb state: Uu, Uw
> ;; in all states: Q
> @@ -585,6 +585,12 @@ (define_memory_constraint "Uz"
> (and (match_code "mem")
> (match_test "arm_coproc_ldc_stc_legitimate_address (op)")))
>
> +(define_memory_constraint "Uo"
> + "@internal
> + A memory operand for Arm/Thumb-2 LDRD/STRD"
> + (and (match_code "mem")
> + (match_test "arm_ldrd_legitimate_address (op)")))
> +
> ;; We used to have constraint letters for S and R in ARM state, but
> ;; all uses of these now appear to have been removed.
>
> diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md
> index 197054b611892006e7df97a4480b349a55a31f07..fca61aeb6997a0dc207ad762097ed1ab65ace642 100644
> --- a/gcc/config/arm/predicates.md
> +++ b/gcc/config/arm/predicates.md
> @@ -849,6 +849,10 @@ (define_predicate "arm_sync_memory_operand"
> (and (match_operand 0 "memory_operand")
> (match_code "reg" "0")))
>
> +;; True if the operand is memory reference suitable for a ldrd/strd.
> +(define_predicate "arm_ldrd_memory_operand"
> + (match_test "arm_ldrd_legitimate_address (op)"))
> +
> ;; Predicates for parallel expanders based on mode.
> (define_special_predicate "vect_par_constant_high"
> (match_code "parallel")
> diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md
> index 0a8347fc5980845642d39bcbe0ef18f97cc446e4..38974fa762683e676d5ec52a7308e9b014c836a2 100644
> --- a/gcc/config/arm/sync.md
> +++ b/gcc/config/arm/sync.md
> @@ -161,7 +161,7 @@ (define_expand "atomic_store<mode>"
> (define_insn "arm_atomic_loaddi2_ldrd"
> [(set (match_operand:DI 0 "register_operand" "=r")
> (unspec_volatile:DI
> - [(match_operand:DI 1 "memory_operand" "m")]
> + [(match_operand:DI 1 "arm_ldrd_memory_operand" "Uo")]
> VUNSPEC_LDRD_ATOMIC))]
> "ARM_DOUBLEWORD_ALIGN && TARGET_HAVE_LPAE"
> "ldrd\t%0, %H0, %1"
> diff --git a/gcc/testsuite/gcc.target/arm/pr117675.c b/gcc/testsuite/gcc.target/arm/pr117675.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..586bc9db90a7613c289df8d48ddd9780b3e434cd
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr117675.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -marm" } */
> +/* { dg-require-effective-target arm_arch_v7ve_neon_ok } */
> +/* { dg-add-options arm_arch_v7ve_neon } */
> +/* { dg-final { check-function-bodies "**" "" "" } } */
> +
> +/*
> +** f1:
> +** add r0, r0, r1
> +** ldrd r0, r1, \[r0\]
> +** bx lr
> +*/
> +long long f1 (char *p, int i)
> +{
> + return __atomic_load_n ((long long *)(p + i), __ATOMIC_RELAXED);
> +}
> +
>
@@ -202,6 +202,7 @@ extern rtx arm_load_tp (rtx);
extern bool arm_coproc_builtin_available (enum unspecv);
extern bool arm_coproc_ldc_stc_legitimate_address (rtx);
extern rtx arm_stack_protect_tls_canary_mem (bool);
+extern bool arm_ldrd_legitimate_address (rtx);
#if defined TREE_CODE
@@ -34481,6 +34481,30 @@ arm_coproc_ldc_stc_legitimate_address (rtx op)
return false;
}
+/* Return true if OP is a valid memory operand for LDRD/STRD without any
+ register overlap restrictions. Allow [base] and [base, imm] for now. */
+bool
+arm_ldrd_legitimate_address (rtx op)
+{
+ if (!MEM_P (op))
+ return false;
+
+ op = XEXP (op, 0);
+ if (REG_P (op))
+ return true;
+
+ if (GET_CODE (op) != PLUS)
+ return false;
+ if (!REG_P (XEXP (op, 0)) || !CONST_INT_P (XEXP (op, 1)))
+ return false;
+
+ HOST_WIDE_INT val = INTVAL (XEXP (op, 1));
+
+ if (TARGET_ARM)
+ return IN_RANGE (val, -255, 255);
+ return IN_RANGE (val, -1020, 1020) && (val & 3) == 0;
+}
+
/* Return the diagnostic message string if conversion from FROMTYPE to
TOTYPE is not allowed, NULL otherwise. */
@@ -39,7 +39,7 @@
;; in all states: Pg
;; The following memory constraints have been used:
-;; in ARM/Thumb-2 state: Uh, Ut, Uv, Uy, Un, Um, Us, Up, Uf, Ux, Ul
+;; in ARM/Thumb-2 state: Uh, Ut, Uv, Uy, Un, Um, Us, Uo, Up, Uf, Ux, Ul, Uz
;; in ARM state: Uq
;; in Thumb state: Uu, Uw
;; in all states: Q
@@ -585,6 +585,12 @@ (define_memory_constraint "Uz"
(and (match_code "mem")
(match_test "arm_coproc_ldc_stc_legitimate_address (op)")))
+(define_memory_constraint "Uo"
+ "@internal
+ A memory operand for Arm/Thumb-2 LDRD/STRD"
+ (and (match_code "mem")
+ (match_test "arm_ldrd_legitimate_address (op)")))
+
;; We used to have constraint letters for S and R in ARM state, but
;; all uses of these now appear to have been removed.
@@ -849,6 +849,10 @@ (define_predicate "arm_sync_memory_operand"
(and (match_operand 0 "memory_operand")
(match_code "reg" "0")))
+;; True if the operand is memory reference suitable for a ldrd/strd.
+(define_predicate "arm_ldrd_memory_operand"
+ (match_test "arm_ldrd_legitimate_address (op)"))
+
;; Predicates for parallel expanders based on mode.
(define_special_predicate "vect_par_constant_high"
(match_code "parallel")
@@ -161,7 +161,7 @@ (define_expand "atomic_store<mode>"
(define_insn "arm_atomic_loaddi2_ldrd"
[(set (match_operand:DI 0 "register_operand" "=r")
(unspec_volatile:DI
- [(match_operand:DI 1 "memory_operand" "m")]
+ [(match_operand:DI 1 "arm_ldrd_memory_operand" "Uo")]
VUNSPEC_LDRD_ATOMIC))]
"ARM_DOUBLEWORD_ALIGN && TARGET_HAVE_LPAE"
"ldrd\t%0, %H0, %1"
new file mode 100644
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -marm" } */
+/* { dg-require-effective-target arm_arch_v7ve_neon_ok } */
+/* { dg-add-options arm_arch_v7ve_neon } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+/*
+** f1:
+** add r0, r0, r1
+** ldrd r0, r1, \[r0\]
+** bx lr
+*/
+long long f1 (char *p, int i)
+{
+ return __atomic_load_n ((long long *)(p + i), __ATOMIC_RELAXED);
+}
+