RISCV fence and load order broken when function inlined twice

[macdice]

struct PgAioHandle {
    char x;
    int y;
};

static inline bool
f(struct PgAioHandle *ioh, int y, char *x)
{
    *x = ioh->x;
    __atomic_thread_fence(__ATOMIC_ACQUIRE);
    if (ioh->y != y)
      return false;
    return true;
}

bool
g(struct PgAioHandle *ioh, int y)
{
    char x;
    if (!f(ioh, y, &x))
        return false;
    if (!f(ioh, y, &x))
        return false;
    return x != 0;
}

If f() is inlined just once (ie commend out the second call), then we load ->x before ->y with a fence as expected:

        lbu     a2, 0(a0)
        fence   r, rw
        lw      a0, 4(a0)

If f() is inlined twice, then we get this:

        fence   r, rw
        lw      a2, 4(a0)
        bne     a2, a1, .LBB0_3
        fence   r, rw
        lw      a2, 4(a0)
        bne     a2, a1, .LBB0_3
        lbu     a0, 0(a0)

The lbu of ->x is on the wrong side of the fence, right? Happens with all Clang versions on godbolt with -O1 and above, but doesn't happen with GCC. (This is minimized from src/backend/storage/aio/aio.c in PostgreSQL.)

https://godbolt.org/z/En4xKcfWz

[llvmbot]

@llvm/issue-subscribers-backend-risc-v

Author: Thomas Munro (macdice)

``` struct PgAioHandle { char x; int y; };

static inline bool
f(struct PgAioHandle *ioh, int y, char *x)
{
*x = ioh->x;
__atomic_thread_fence(__ATOMIC_ACQUIRE);
if (ioh->y != y)
return false;
return true;
}

bool
g(struct PgAioHandle *ioh, int y)
{
char x;
if (!f(ioh, y, &x))
return false;
if (!f(ioh, y, &x))
return false;
return x != 0;
}

If f() is inlined just once (ie commend out the second call), then we load ->x before ->y with a fence as expected:

    lbu     a2, 0(a0)
    fence   r, rw
    lw      a0, 4(a0)

If f() is inlined twice, then we get this:

    fence   r, rw
    lw      a2, 4(a0)
    bne     a2, a1, .LBB0_3
    fence   r, rw
    lw      a2, 4(a0)
    bne     a2, a1, .LBB0_3
    lbu     a0, 0(a0)

The lbu of ->x is on the wrong side of the fence, right?  Happens with all Clang versions on godbolt with -O1 and above, but doesn't happen with GCC.  (This is minimized from src/backend/storage/aio/aio.c in PostgreSQL.)

https://godbolt.org/z/En4xKcfWz

</details>

[fioraking]

An acquire fence only constrains the reordering of memory operations after the fence; operations before the fence are allowed to be delayed or moved past it.

[macdice]

Hmm. Isn't acquire supposed to prevent load-load reordering too? Anyway, even __ATOMIC_SEQ_CST exhibits the problem. Is it possible that the compiler barrier contained in the fence is being lost, or something like that? The problem goes away if I do any one of these:

• additionally: __atomic_signal_fence(__ATOMIC_ACQUIRE)
• additionally: __asm__ __volatile__("" ::: "memory")
• add volatile qualifier to those member variables
• change the target to ARM

The first two being just explicit ways to ask for a compiler barrier, which I was expecting to be included in __atomic_thread_fence() already.

Our actual model is not really acquire and release fence (RISCV: FENCE R, RW and FENCE RW, W), it's read and write barriers (RISCV: FENCE R, R and FENCE W, W). We could just hand-roll that asm as we do on other systems, it's just that we fall back to GCC/Clang acq/rel on platforms where we don't have that. We expected those to be stronger. Is that actually incorrect? Why would only Clang + RISCV be affected?

[topperc]

Looks like the MachineSink pass is moving the load past the FENCE. The FENCE instructions doesn't have the mayStore attribute so the sinking isn't stopped. I'm not sure if adding mayStore to FENCE is the correct fix or if there is some other way to tell MachineSink not to do this.