On 27/06/2024 17:16, Wilco Dijkstra wrote:
> Hi Richard,
>
>> Doing just this will mean that the register allocator will have to undo a
>> pre/post memory operand that was accepted by the predicate (memory_operand).
>> I think we really need a tighter predicate (lets call it noautoinc_mem_op)
>> here to avoid that. Note that the existing uses of Uw also had another
>> alternative that did permit 'm', so this wasn't previously practical, but
>> they had alternative ways of being reloaded.
>>
>> No, sorry that won't work; there's another 'm' alternative here as well.
>> The correct fix is to add alternatives for T1, I think, similar to the one
>> in thumb1_movsi_insn.
>>
>> Also, by observation I think there's a similar problem in the load
>> operations.
>
> Just using 'Uw' works fine, but restricting the memory operand too is better
> indeed.
> I added 'restricted_memory_operand' that only disallows Thumb-1 postincrement.
>
> There were also a few more cases in unaligned accesses where 'm' was used
> incorrectly when
> emitting Thumb-1 LDR/STR alternatives (and where no LDM/STMis allowed), so
> those also use
> 'Uw' and 'restricted_memory_operand'.
>
> Long term it seems like a better idea is to remove support this odd
> post-increment
> in general memory operand and only emit it from a peephole pass.
>
> Cheers,
> Wilco
>
>
> v3: Use 'Uw' in a few more cases. Add 'restricted_memory_operand'.
>
> A Thumb-1 memory operand allows single-register LDMIA/STMIA. This doesn't get
> printed as LDR/STR with writeback in unified syntax, resulting in strange
> assembler errors if writeback is selected. To work around this, use the 'Uw'
> constraint that blocks writeback. Also use a new 'restricted_memory_operand'
> which is a general memory operand that disallows writeback in Thumb-1.
> A few other patterns were using 'm' for Thumb-1 in a similar way, update these
> to also use 'restricted_memory_operand' and 'Uw'.
>
> Passes bootstrap & regress, OK for commit (and backport to GCC14.2)?
I'm not a major fan of the name restricted_memory_operand as it doesn't
describe which restriction is being applied and something like
t1_restricted_memory_operand would not be any clearer. Perhaps
mem_and_no_t1_wback_op would be better?
OK with that change.
R.
>
> gcc:
> PR target/115188
> * config/arm/arm.md (unaligned_loadsi): Use 'Uw' constraint and
> 'restricted_memory_operand'.
> (unaligned_loadhiu): Likewise.
> (unaligned_storesi): Likewise.
> (unaligned_storehi): Likewise.
> * config/arm/predicates.md (restricted_memory_operand): Add new
> predicate.
> * config/arm/sync.md (arm_atomic_load<mode>): Use 'Uw' constraint.
> (arm_atomic_store<mode>): Likewise.
>
> gcc/testsuite:
> PR target/115188
> * gcc.target/arm/pr115188.c: Add new test.
>
> ---
>
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index
> f47e036a8034ed16c61bbd753c7a7cd3efb1ecbd..c962a9341779e4da38f4e1afb26d4a364fc5aee4
> 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -5011,7 +5011,7 @@
>
> (define_insn "unaligned_loadsi"
> [(set (match_operand:SI 0 "s_register_operand" "=l,l,r")
> - (unspec:SI [(match_operand:SI 1 "memory_operand" "m,Uw,m")]
> + (unspec:SI [(match_operand:SI 1 "restricted_memory_operand" "Uw,Uw,m")]
> UNSPEC_UNALIGNED_LOAD))]
> "unaligned_access"
> "@
> @@ -5041,7 +5041,7 @@
> (define_insn "unaligned_loadhiu"
> [(set (match_operand:SI 0 "s_register_operand" "=l,l,r")
> (zero_extend:SI
> - (unspec:HI [(match_operand:HI 1 "memory_operand" "m,Uw,m")]
> + (unspec:HI [(match_operand:HI 1 "restricted_memory_operand"
> "Uw,Uw,m")]
> UNSPEC_UNALIGNED_LOAD)))]
> "unaligned_access"
> "@
> @@ -5066,7 +5066,7 @@
> (set_attr "type" "store_8")])
>
> (define_insn "unaligned_storesi"
> - [(set (match_operand:SI 0 "memory_operand" "=m,Uw,m")
> + [(set (match_operand:SI 0 "restricted_memory_operand" "=Uw,Uw,m")
> (unspec:SI [(match_operand:SI 1 "s_register_operand" "l,l,r")]
> UNSPEC_UNALIGNED_STORE))]
> "unaligned_access"
> @@ -5081,7 +5081,7 @@
> (set_attr "type" "store_4")])
>
> (define_insn "unaligned_storehi"
> - [(set (match_operand:HI 0 "memory_operand" "=m,Uw,m")
> + [(set (match_operand:HI 0 "restricted_memory_operand" "=Uw,Uw,m")
> (unspec:HI [(match_operand:HI 1 "s_register_operand" "l,l,r")]
> UNSPEC_UNALIGNED_STORE))]
> "unaligned_access"
> diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md
> index
> 4994c0c57d6431117c16f7a05e800821dee93408..3dfe381c098c06517dca6026f8dafe87b46135ae
> 100644
> --- a/gcc/config/arm/predicates.md
> +++ b/gcc/config/arm/predicates.md
> @@ -907,3 +907,8 @@
> ;; A special predicate that doesn't match a particular mode.
> (define_special_predicate "arm_any_register_operand"
> (match_code "reg"))
> +
> +;; General memory operand that disallows Thumb-1 POST_INC.
> +(define_predicate "restricted_memory_operand"
> + (and (match_operand 0 "memory_operand")
> + (match_test "!(TARGET_THUMB1 && GET_CODE (XEXP (op, 0)) ==
> POST_INC)")))
> diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md
> index
> df8dbe170cacb6b60d56a6f19aadd5a6c9c51f7a..7696c1a6f9819cfcbc9008f58431b9c2f08cb0ce
> 100644
> --- a/gcc/config/arm/sync.md
> +++ b/gcc/config/arm/sync.md
> @@ -65,7 +65,7 @@
> (define_insn "arm_atomic_load<mode>"
> [(set (match_operand:QHSI 0 "register_operand" "=r,l")
> (unspec_volatile:QHSI
> - [(match_operand:QHSI 1 "memory_operand" "m,m")]
> + [(match_operand:QHSI 1 "restricted_memory_operand" "m,Uw")]
> VUNSPEC_LDR))]
> ""
> "ldr<sync_sfx>\t%0, %1"
> @@ -81,7 +81,7 @@
> )
>
> (define_insn "arm_atomic_store<mode>"
> - [(set (match_operand:QHSI 0 "memory_operand" "=m,m")
> + [(set (match_operand:QHSI 0 "restricted_memory_operand" "=m,Uw")
> (unspec_volatile:QHSI
> [(match_operand:QHSI 1 "register_operand" "r,l")]
> VUNSPEC_STR))]
> diff --git a/gcc/testsuite/gcc.target/arm/pr115188.c
> b/gcc/testsuite/gcc.target/arm/pr115188.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..9a4022b56796d6962bb3f22e40bac4b81eb78ccf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr115188.c
> @@ -0,0 +1,10 @@
> +/* { dg-do assemble } */
> +/* { dg-require-effective-target arm_arch_v6m_ok }
> +/* { dg-options "-O2" } */
> +/* { dg-add-options arm_arch_v6m } */
> +
> +void init (int *p, int n)
> +{
> + for (int i = 0; i < n; i++)
> + __atomic_store_4 (p + i, 0, __ATOMIC_RELAXED);
> +}
>
