| Message ID | PAWPR08MB8982355710EAFBB5EBB593CB83C72@PAWPR08MB8982.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Arm: Fix disassembly error in Thumb-1 relaxed load/store [PR115188] | expand |
On 11/06/2024 17:35, Wilco Dijkstra wrote: > Hi Christophe, > >> PR target/115153 > I guess this is typo (should be 115188) ? > > Correct. > >> +/* { dg-options "-O2 -mthumb" } */-mthumb is included in arm_arch_v6m, so I think you don't need to add it > here? > > Indeed, it's not strictly necessary. Fixed in v2: > > 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. 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. R. > > Passes bootstrap & regress, OK for commit and backport? > > gcc: > PR target/115188 > * 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/sync.md b/gcc/config/arm/sync.md > index df8dbe170cacb6b60d56a6f19aadd5a6c9c51f7a..e856ee51d9ae7b945c4d1e9d1f08afeedc95707a 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 "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 "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); > +} >
On 12/06/2024 11:35, Richard Earnshaw (lists) wrote: > On 11/06/2024 17:35, Wilco Dijkstra wrote: >> Hi Christophe, >> >>> PR target/115153 >> I guess this is typo (should be 115188) ? >> >> Correct. >> >>> +/* { dg-options "-O2 -mthumb" } */-mthumb is included in arm_arch_v6m, so I think you don't need to add it >> here? >> >> Indeed, it's not strictly necessary. Fixed in v2: >> >> 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. > > 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. R. > > R. > >> >> Passes bootstrap & regress, OK for commit and backport? >> >> gcc: >> PR target/115188 >> * 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/sync.md b/gcc/config/arm/sync.md >> index df8dbe170cacb6b60d56a6f19aadd5a6c9c51f7a..e856ee51d9ae7b945c4d1e9d1f08afeedc95707a 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 "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 "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); >> +} >> >
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<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); +}
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); > +} >
diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md index df8dbe170cacb6b60d56a6f19aadd5a6c9c51f7a..e856ee51d9ae7b945c4d1e9d1f08afeedc95707a 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 "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 "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); +}