Re: [PATCH v3] Arm: Fix disassembly error in Thumb-1 relaxed load/store [PR115188]
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): Use 'Uw' constraint. > (arm_atomic_store): 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"
Re: [PATCH v3] Arm: Fix disassembly error in Thumb-1 relaxed load/store [PR115188]
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)? 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): Use 'Uw' constraint. (arm_atomic_store): 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" [(set (m
