[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #13 from Patrick O'Neill --- Created attachment 56487 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56487=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
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #12 from Rory Bolt --- (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...
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #11 from Patrick O'Neill --- 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
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #10 from Rory Bolt --- Tested and verified on little endian too.
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #9 from Rory Bolt --- Created attachment 55153 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55153=edit patch Tested fix for big endian, NOT tested on little endian
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #8 from Rory Bolt --- 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.
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
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.
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #6 from Rory Bolt --- 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!
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #5 from Patrick O'Neill --- 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!
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #4 from Patrick O'Neill --- Sorry about the nonsense comment, I clicked enter too early. I'm taking a look at this.
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 Patrick O'Neill changed: What|Removed |Added CC||patrick at rivosinc dot com --- Comment #3 from Patrick O'Neill --- The recent patches just copied the libatomic code inline
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 Richard Biener changed: What|Removed |Added Ever confirmed|0 |1 Last reconfirmed||2023-05-23 Status|UNCONFIRMED |NEW
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #2 from Andrew Pinski --- So it looks like it is still broken. andia4,s0,3 sllia4,a4,3 li a2,1 andia3,s0,-4 sll a2,a2,a4 amoor.w.aqrla5,a2,0(a3) andia4,s0,3 sllia4,a4,3 li a2,1 andia3,s0,-4 sll a2,a2,a4 amoor.w.aqrla5,a2,0(a3)
[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109933 --- Comment #1 from Andrew Pinski --- This might be fixed on the trunk with recent patches; note those patches are NOT backportable from what I remember.
