Hi Jeff, Thanks for taking the time once again to review the patch.
On 7/10/26 20:09, Jeffrey Law wrote: > On 6/19/2026 6:04 AM, Aleksa Paunovic wrote: >> From: Chao-ying Fu<[email protected]> >> >> Ensure the proper sub-word address is computed >> when using built-in atomics on big-endian RISC-V targets. >> >> Without this change, multiple tests were failing, including: >> libatomic.c/atomic-compare-exchange-1.c >> libatomic.c/atomic-compare-exchange-2.c >> libatomic.c/atomic-exchange-1.c >> libatomic.c/atomic-exchange-2.c >> libatomic.c/atomic-op-1.c >> libatomic.c/atomic-op-2.c >> libatomic.c/generic-2.c >> gcc.target/riscv/amo/inline-atomics-3.c >> gcc.target/riscv/amo/inline-atomics-4.c >> gcc.target/riscv/amo/inline-atomics-5.c >> gcc.target/riscv/amo/inline-atomics-6.c >> gcc.target/riscv/amo/inline-atomics-7.c >> gcc.target/riscv/amo/inline-atomics-8.c >> >> The patch was tested using QEMU modified to run big-endian RISC-V >> executables [1]. >> >> [1]https://github.com/djtodoro/qemu/tree/riscvbe/current# >> >> Signed-off-by: Aleksa Paunovic<[email protected]> >> >> gcc/ChangeLog: >> >> * config/riscv/riscv.cc (riscv_subword_address): Add emit_move_insn >> for big-endian. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/riscv/amo/big-endian-subword-amo-hi.c: New test. >> * gcc.target/riscv/amo/big-endian-subword-amo-qi.c: New test. >> --- >> Changes in v4: >> - Drop the second and the third patches. >> - XFAIL the tests if RVV is enabled on big-endian builds instead of skipping. >> - Compile the tests with -O0 to avoid potential DCE issues. >> - Link to >> v3:https://patchwork.sourceware.org/project/gcc/cover/[email protected]/ > So there continues to be meaningful pushback from the wider community on > big-endian efforts. So this is likely going to continue to be very low > priority across the various tools. > I think the main pushback has come from the Linux kernel community. In fact, I believe we've had meaningful success getting big-endian changes upstreamed. Even in the case where certain changes were not accepted, usually it was related to needing kernel community approval first (userspace QEMU). A few examples: - QEMU big-endian support [1] - RISC-V psABI (not upstream yet, but we've made good progress) [2] - LLVM [3] > >> >> --- >> gcc/config/riscv/riscv.cc | 5 +++++ >> .../gcc.target/riscv/amo/big-endian-subword-amo-hi.c | 10 ++++++++++ >> .../gcc.target/riscv/amo/big-endian-subword-amo-qi.c | 10 ++++++++++ >> 3 files changed, 25 insertions(+) >> create mode 100644 >> gcc/testsuite/gcc.target/riscv/amo/big-endian-subword-amo-hi.c >> create mode 100644 >> gcc/testsuite/gcc.target/riscv/amo/big-endian-subword-amo-qi.c >> >> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc >> index bd0046ad2e8..f0a6e43a182 100644 >> --- a/gcc/config/riscv/riscv.cc >> +++ b/gcc/config/riscv/riscv.cc >> @@ -11969,6 +11969,11 @@ riscv_subword_address (rtx mem, rtx *aligned_mem, >> rtx *shift, rtx *mask, >> /* Calculate the shift amount. */ >> emit_move_insn (*shift, gen_rtx_AND (SImode, gen_lowpart (SImode, addr), >> gen_int_mode (3, SImode))); >> + if (TARGET_BIG_ENDIAN) >> + emit_move_insn (*shift, gen_rtx_XOR (SImode, *shift, >> + gen_int_mode (GET_MODE (mem) == QImode >> + ? 3 : 2, SImode))); > So your xor constant is 3 for QI and 2 for HI. So for QI that means > 3<->0, 1<->2 and for HI it just flips between 0<->1. That seems sensible. > > Presumably we're assuming the right thing is going to happen by using > the W forms on rv64 and thus we'll never get into this routine with an > SImode object. > >> +/* { dg-do compile } */ >> +/* { dg-options "-mbig-endian -O0" } */ > So I would generally advise against -O0 in here and instead let the > testsuite iterate through the various optimization options. You may > need to improve your scan regexp, or it may work as-is. And you should > probably be using dg-additional-options to inject the -mbig-endian > rather than dg-options. Fixed in v5. Thanks! >> +/* { dg-xfail-if "RVV is currently broken on big-endian RISC-V builds" { >> riscv*-*-* } { "-march=rv*v*" } } */ > Yea, not sure how best to handle this, but this seems like it's likely > to be fragile with the -march matching. Perhaps just let it fail > without marking it as xfail? This stuff is all well outside of what the > user is going to see and 99% of what developers will see, so I could > leave with a hard dejagnu FAIL here. Thoughts? Agreed! Fixed in v5. Best regards, Aleksa Links: [1] https://patchew.org/QEMU/[email protected]/ [2] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/470 [3] https://github.com/llvm/llvm-project/pull/180912
