[Bug target/113282] New: RISC-V non-atomic union store/load reordering

[Bug target/113282] New: RISC-V non-atomic union store/load reordering

[patrick at rivosinc dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113282

            Bug ID: 113282
           Summary: RISC-V non-atomic union store/load reordering
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: patrick at rivosinc dot com
  Target Milestone: ---

I'm not sure if this is a bug or a valid optimization. Posting this here to
educate myself :)

Testcase:
union {
  long c;
  int b;
} e;
int f;
int *g = &e.b;
long *h = &e.c;
int main() {
  *g = 1;
  if (*h == 1)
    return 0;
  else
    return 1;
}

risc-v -O2:
main:
        lui     a5,%hi(h)
        ld      a5,%lo(h)(a5)
        ld      a0,0(a5)
        lui     a5,%hi(g)
        ld      a5,%lo(g)(a5) << Load
        li      a4,1
        addi    a0,a0,-1
        snez    a0,a0
        sw      a4,0(a5) << Store pushed below the load
        ret
f:
        .zero   4
        .zero   4
e:
        .zero   8
h:
        .dword  e
g:
        .dword  e

My educated guess is that since the operations aren't marked as atomic, the
compiler is free to ignore the dependency and optimize the load before the
store. After marking the ops as atomic:
union {
  long c;
  int b;
} e;
int f;
int *g = &e.b;
long *h = &e.c;
int main() {
  __atomic_store_n(g, 1, __ATOMIC_RELAXED);
  if (__atomic_load_n(h, __ATOMIC_RELAXED) == 1)
    return 0;
  else
    return 1;
}

The problem goes away:
main:
        lui     a5,%hi(g)
        ld      a5,%lo(g)(a5)
        li      a4,1
        sw      a4,0(a5) << Store remains above the load
        lui     a5,%hi(h)
        ld      a5,%lo(h)(a5)
        ld      a0,0(a5) << Load
        addi    a0,a0,-1
        snez    a0,a0
        ret
f:
        .zero   4
        .zero   4
e:
        .zero   8
h:
        .dword  e
g:
        .dword  e

Is that a correct line of reasoning? On x86 this problem doesn't manifest so
it's also possibly a risc-v bug.

[Bug target/113282] RISC-V non-atomic union store/load reordering

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113282

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
           Keywords|                            |alias
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Int and long does not alias so moving the store past the load is a valid thing
to do. Note the union does not make a difference here since it is not seen due
taking the address of the fields.

[Bug target/113282] RISC-V non-atomic union store/load reordering

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113282

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
> On x86 this problem

Yes x86 does not enable the scheduler before ra and has less registers so the
scheduler is not as aggressive as on the other targets.

Note this is standard strict aliasing issue even. Which is definitely mentioned
as a non issue even.

[Bug target/113282] RISC-V non-atomic union store/load reordering

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113282

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Note -fno-strict-aliasing will allow the code to work the way you want it to
work.

[Bug target/113282] RISC-V non-atomic union store/load reordering

[patrick at rivosinc dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113282

Patrick O'Neill <patrick at rivosinc dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |FIXED

--- Comment #4 from Patrick O'Neill <patrick at rivosinc dot com> ---
Thanks for the explanation!
I didn't know about strict aliasing :)

[Bug target/113282] RISC-V non-atomic union store/load reordering

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113282

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |INVALID

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
It is even mentioned here:
https://gcc.gnu.org/bugs/#nonbugs

....