Re: [PATCH v2] Arm: Fix disassembly error in Thumb-1 relaxed load/store [PR115188]
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): 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/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"
>>[(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\t%0, %1"
>> @@ -81,7 +81,7 @@
>> )
>>
>> (define_insn "arm_atomic_store"
>> - [(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
>> ..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);
>> +}
>>
>
Re: [PATCH v2] Arm: Fix disassembly error in Thumb-1 relaxed load/store [PR115188]
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): 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/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"
>[(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\t%0, %1"
> @@ -81,7 +81,7 @@
> )
>
> (define_insn "arm_atomic_store"
> - [(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
> ..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);
> +}
>
[PATCH v2] Arm: Fix disassembly error in Thumb-1 relaxed load/store [PR115188]
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.
Passes bootstrap & regress, OK for commit and backport?
gcc:
PR target/115188
* 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/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"
[(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\t%0, %1"
@@ -81,7 +81,7 @@
)
(define_insn "arm_atomic_store"
- [(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
..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);
+}
