* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
@ 2023-05-22 18:58 ` pinskia at gcc dot gnu.org
2023-05-22 19:02 ` pinskia at gcc dot gnu.org
` (12 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-05-22 18:58 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This might be fixed on the trunk with recent patches; note those patches are
NOT backportable from what I remember.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
2023-05-22 18:58 ` [Bug target/109933] " pinskia at gcc dot gnu.org
@ 2023-05-22 19:02 ` pinskia at gcc dot gnu.org
2023-05-23 8:06 ` rguenth at gcc dot gnu.org
` (11 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-05-22 19:02 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
So it looks like it is still broken.
andi a4,s0,3
slli a4,a4,3
li a2,1
andi a3,s0,-4
sll a2,a2,a4
amoor.w.aqrl a5,a2,0(a3)
andi a4,s0,3
slli a4,a4,3
li a2,1
andi a3,s0,-4
sll a2,a2,a4
amoor.w.aqrl a5,a2,0(a3)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
2023-05-22 18:58 ` [Bug target/109933] " pinskia at gcc dot gnu.org
2023-05-22 19:02 ` pinskia at gcc dot gnu.org
@ 2023-05-23 8:06 ` rguenth at gcc dot gnu.org
2023-05-23 22:00 ` patrick at rivosinc dot com
` (10 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-05-23 8:06 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
Richard Biener <rguenth at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Ever confirmed|0 |1
Last reconfirmed| |2023-05-23
Status|UNCONFIRMED |NEW
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (2 preceding siblings ...)
2023-05-23 8:06 ` rguenth at gcc dot gnu.org
@ 2023-05-23 22:00 ` patrick at rivosinc dot com
2023-05-23 22:02 ` patrick at rivosinc dot com
` (9 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: patrick at rivosinc dot com @ 2023-05-23 22:00 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
Patrick O'Neill <patrick at rivosinc dot com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |patrick at rivosinc dot com
--- Comment #3 from Patrick O'Neill <patrick at rivosinc dot com> ---
The recent patches just copied the libatomic code inline
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (3 preceding siblings ...)
2023-05-23 22:00 ` patrick at rivosinc dot com
@ 2023-05-23 22:02 ` patrick at rivosinc dot com
2023-05-23 22:49 ` patrick at rivosinc dot com
` (8 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: patrick at rivosinc dot com @ 2023-05-23 22:02 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #4 from Patrick O'Neill <patrick at rivosinc dot com> ---
Sorry about the nonsense comment, I clicked enter too early. I'm taking a look
at this.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (4 preceding siblings ...)
2023-05-23 22:02 ` patrick at rivosinc dot com
@ 2023-05-23 22:49 ` patrick at rivosinc dot com
2023-05-23 23:50 ` rory.bolt at gmail dot com
` (7 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: patrick at rivosinc dot com @ 2023-05-23 22:49 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #5 from Patrick O'Neill <patrick at rivosinc dot com> ---
I don't have a big-endian environment set up to validate the issue/test a fix
and I likely won't be able to get to this for a while.
The relevant code is here (untouched by the recent patches):
Trunk
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/sync.md;h=9fc626267de3840ce15d196bff72440a980fd234;hb=HEAD#l535
GCC 12
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/sync.md;h=86b41e6b00a9331ed4b46e73cc66c102c6d2d981;hb=2ee5e4300186a92ad73f1a1a64cb918dc76c8d67#l155
The main difference from 12->trunk in this testcase is that AMOOR now uses
.aqrl instead of a fence. That does not affect this testcase, so a fix on trunk
should be applicable to the 12 branch (and vice-versa).
Thanks for the bug report!
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (5 preceding siblings ...)
2023-05-23 22:49 ` patrick at rivosinc dot com
@ 2023-05-23 23:50 ` rory.bolt at gmail dot com
2023-05-23 23:58 ` palmer at gcc dot gnu.org
` (6 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: rory.bolt at gmail dot com @ 2023-05-23 23:50 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #6 from Rory Bolt <rory.bolt at gmail dot com> ---
Ah... that code makes so much sense now...
So my original comment about simply using a different constant was too
simplistic; what is being attempted is to shift the constant 1 into the correct
byte position since the flag is only a single byte but the atomic store is done
on a word... so the shift logic will need to be rewritten for big endian
targets. This also explains the masking of the low order address bits...
Interesting!
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (6 preceding siblings ...)
2023-05-23 23:50 ` rory.bolt at gmail dot com
@ 2023-05-23 23:58 ` palmer at gcc dot gnu.org
2023-05-24 14:08 ` rory.bolt at gmail dot com
` (5 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: palmer at gcc dot gnu.org @ 2023-05-23 23:58 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
palmer at gcc dot gnu.org changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |palmer at gcc dot gnu.org
--- Comment #7 from palmer at gcc dot gnu.org ---
(In reply to Rory Bolt from comment #6)
> Ah... that code makes so much sense now...
>
> So my original comment about simply using a different constant was too
> simplistic; what is being attempted is to shift the constant 1 into the
> correct byte position since the flag is only a single byte but the atomic
> store is done on a word... so the shift logic will need to be rewritten for
> big endian targets. This also explains the masking of the low order address
> bits...
>
> Interesting!
That seems likely to be the culprit. Do you have time to send a patch?
We should probably also poke through the other sub-word patterns and make sure
nothing else got dropped for BE.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (7 preceding siblings ...)
2023-05-23 23:58 ` palmer at gcc dot gnu.org
@ 2023-05-24 14:08 ` rory.bolt at gmail dot com
2023-05-25 2:39 ` rory.bolt at gmail dot com
` (4 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: rory.bolt at gmail dot com @ 2023-05-24 14:08 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #8 from Rory Bolt <rory.bolt at gmail dot com> ---
So...
The logic for this is simple:
For little endian the shift amount is ((address & 3) * 8)
For big endian the shift amount is ((3 -(address & 3)) * 8)
Unfortunately I have ZERO experience modifying GCC, and the mechanism to
determine if it is generating big endian code or little endian code is not
obvious to me...
So working on this in my spare time it will be a while for me to create a
patch. That said, I do have a full big endian linux environment so I can test a
patch (relatively quickly - it takes a while to build GCC ;-)) if some one
beats me to this.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (8 preceding siblings ...)
2023-05-24 14:08 ` rory.bolt at gmail dot com
@ 2023-05-25 2:39 ` rory.bolt at gmail dot com
2023-05-25 16:46 ` rory.bolt at gmail dot com
` (3 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: rory.bolt at gmail dot com @ 2023-05-25 2:39 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #9 from Rory Bolt <rory.bolt at gmail dot com> ---
Created attachment 55153
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55153&action=edit
patch
Tested fix for big endian, NOT tested on little endian
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (9 preceding siblings ...)
2023-05-25 2:39 ` rory.bolt at gmail dot com
@ 2023-05-25 16:46 ` rory.bolt at gmail dot com
2023-05-25 18:30 ` patrick at rivosinc dot com
` (2 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: rory.bolt at gmail dot com @ 2023-05-25 16:46 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #10 from Rory Bolt <rory.bolt at gmail dot com> ---
Tested and verified on little endian too.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (10 preceding siblings ...)
2023-05-25 16:46 ` rory.bolt at gmail dot com
@ 2023-05-25 18:30 ` patrick at rivosinc dot com
2023-05-25 19:10 ` rory.bolt at gmail dot com
2023-11-01 23:55 ` patrick at rivosinc dot com
13 siblings, 0 replies; 15+ messages in thread
From: patrick at rivosinc dot com @ 2023-05-25 18:30 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #11 from Patrick O'Neill <patrick at rivosinc dot com> ---
I can confirm that your fix does *not* break the testsuite on little endian.
We also recently added inline subword atomics to GCC 13, will this code also
need to change for big endian?
Trunk:
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/riscv.cc;h=09fc9e5d95e611f94bc05b4851fef6f50a651c28;hb=HEAD#l7439
GCC-13 branch:
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/riscv.cc;h=5f44f6dc5c9c4ad2416e4195570473ab49c2e535;hb=6506590e70e57ed8d7fb68ab9443e31c31208fb0#l7146
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (11 preceding siblings ...)
2023-05-25 18:30 ` patrick at rivosinc dot com
@ 2023-05-25 19:10 ` rory.bolt at gmail dot com
2023-11-01 23:55 ` patrick at rivosinc dot com
13 siblings, 0 replies; 15+ messages in thread
From: rory.bolt at gmail dot com @ 2023-05-25 19:10 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #12 from Rory Bolt <rory.bolt at gmail dot com> ---
(In reply to Patrick O'Neill from comment #11)
> I can confirm that your fix does *not* break the testsuite on little endian.
>
> We also recently added inline subword atomics to GCC 13, will this code also
> need to change for big endian?
>
> Trunk:
> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/riscv.cc;
> h=09fc9e5d95e611f94bc05b4851fef6f50a651c28;hb=HEAD#l7439
>
> GCC-13 branch:
> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/riscv.cc;
> h=5f44f6dc5c9c4ad2416e4195570473ab49c2e535;
> hb=6506590e70e57ed8d7fb68ab9443e31c31208fb0#l7146
It looks like the same patch will be required there...
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
2023-05-22 18:56 [Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets rory.bolt at gmail dot com
` (12 preceding siblings ...)
2023-05-25 19:10 ` rory.bolt at gmail dot com
@ 2023-11-01 23:55 ` patrick at rivosinc dot com
13 siblings, 0 replies; 15+ messages in thread
From: patrick at rivosinc dot com @ 2023-11-01 23:55 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933
--- Comment #13 from Patrick O'Neill <patrick at rivosinc dot com> ---
Created attachment 56487
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56487&action=edit
Proposed fix
Hi Rory,
Sorry that this slipped off my plate for way too long.
I just got around to refactoring the surrounding code on trunk and revised your
patch to fix this for both test_and_set along with inline subword amo
sequences.
When you have the time can you please test this using your big-endian setup?
I'll handle the little-endian testing.
Patrick
^ permalink raw reply [flat|nested] 15+ messages in thread