[RISCV][MachineScheduler] RISCV backend mis-schedules rdcycle instruction leading to incorrect cycle count

[jupiter-zzp]

https://godbolt.org/z/GfPenPv3f
I've found an issue with the RISCV backend in LLVM's instruction scheduling. The rdcycle instruction is being scheduled incorrectly, which results in an incorrect cycle count calculation.

A small case is:

#include <stdio.h>

int test(int a[]) {
  unsigned long begin_cycle, end_cycle;
  __asm volatile("rdcycle %0"
                    : "=r"(begin_cycle)                                \
                    :                                          \
                    : "memory");
  
  int res = 0;
  for (int i = 0; i < 4; i++) {
    res += a[i];
  }

  __asm volatile("rdcycle %0"
                    : "=r"(end_cycle)                                \
                    :                                          \
                    : "memory");

  printf("cycle = %ld\n", end_cycle - begin_cycle);
  return res;
}

Compiled with -O3:

test:
        addi    sp, sp, -16
        sd      ra, 8(sp)
        sd      s0, 0(sp)
        rdcycle a1
        lw      a2, 0(a0)
        lw      a3, 4(a0)
        lw      a4, 8(a0)
        lw      a0, 12(a0)
        rdcycle a5
        add     a2, a2, a3
        add     a0, a0, a4
        addw    s0, a0, a2
        sub     a1, a5, a1
.Lpcrel_hi0:
        auipc   a0, %pcrel_hi(.L.str)
        addi    a0, a0, %pcrel_lo(.Lpcrel_hi0)
        call    printf
        mv      a0, s0
        ld      ra, 8(sp)
        ld      s0, 0(sp)
        addi    sp, sp, 16
        ret

The second rdcycle instruction is scheduled and cycle count of the for loop is wrong.

[llvmbot]

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

Author: None (jupiter-zzp)

https://godbolt.org/z/GfPenPv3f I've found an issue with the RISCV backend in LLVM's instruction scheduling. The rdcycle instruction is being scheduled incorrectly, which results in an incorrect cycle count calculation.

A small case is:

#include <stdio.h>

int test(int a[]) {
  unsigned long begin_cycle, end_cycle;
  __asm volatile("rdcycle %0"
                    : "=r"(begin_cycle)                                \
                    :                                          \
                    : "memory");
  
  int res = 0;
  for (int i = 0; i < 4; i++) {
    res += a[i];
  }

  __asm volatile("rdcycle %0"
                    : "=r"(end_cycle)                                \
                    :                                          \
                    : "memory");

  printf("cycle = %ld\n", end_cycle - begin_cycle);
  return res;
}

Compiled with -O3:

test:
        addi    sp, sp, -16
        sd      ra, 8(sp)
        sd      s0, 0(sp)
        rdcycle a1
        lw      a2, 0(a0)
        lw      a3, 4(a0)
        lw      a4, 8(a0)
        lw      a0, 12(a0)
        rdcycle a5
        add     a2, a2, a3
        add     a0, a0, a4
        addw    s0, a0, a2
        sub     a1, a5, a1
.Lpcrel_hi0:
        auipc   a0, %pcrel_hi(.L.str)
        addi    a0, a0, %pcrel_lo(.Lpcrel_hi0)
        call    printf
        mv      a0, s0
        ld      ra, 8(sp)
        ld      s0, 0(sp)
        addi    sp, sp, 16
        ret

The second rdcycle instruction is scheduled and cycle count of the for loop is wrong.

[michaelmaitland]

We can probably mark this instruction as isBarrier=1 to fix this problem. I am not sure if it makes sense to set this for all CSRRS instruction or just CSRRS GPR$:rd, CYCLE.Encoding, X0

[topperc]

We can probably mark this instruction as isBarrier=1 to fix this problem. I am not sure if it makes sense to set this for all CSRRS instruction or just CSRRS GPR$:rd, CYCLE.Encoding, X0

The compiler doesn't read the contents of inline assembly strings. We don't know its a rdcycle.

[topperc]

I don't think inline assembly makes any guarantees for what you're trying to do here. You can fix it by passing res as input to the second rdcycle asm block. https://godbolt.org/z/7GeE9o5Ev That will guarantee it is computed before the asm block.

This description from gcc's documentation is relevant. https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile-1

Note that the compiler can move even volatile asm instructions relative to other code, including across jump instructions. For example, on many targets there is a system register that controls the rounding mode of floating-point operations. Setting it with a volatile asm statement, as in the following PowerPC example, does not work reliably.

asm volatile("mtfsf 255, %0" : : "f" (fpenv));
sum = x + y;
The compiler may move the addition back before the volatile asm statement. To make it work as expected, add an artificial dependency to the asm by referencing a variable in the subsequent code, for example:

asm volatile ("mtfsf 255,%1" : "=X" (sum) : "f" (fpenv));
sum = x + y;

[jupiter-zzp]

I don't think inline assembly makes any guarantees for what you're trying to do here. You can fix it by passing res as input to the second rdcycle asm block. https://godbolt.org/z/7GeE9o5Ev That will guarantee it is computed before the asm block.

This description from gcc's documentation is relevant. https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile-1

Note that the compiler can move even volatile asm instructions relative to other code, including across jump instructions. For example, on many targets there is a system register that controls the rounding mode of floating-point operations. Setting it with a volatile asm statement, as in the following PowerPC example, does not work reliably.

asm volatile("mtfsf 255, %0" : : "f" (fpenv));
sum = x + y;
The compiler may move the addition back before the volatile asm statement. To make it work as expected, add an artificial dependency to the asm by referencing a variable in the subsequent code, for example:

asm volatile ("mtfsf 255,%1" : "=X" (sum) : "f" (fpenv));
sum = x + y;

This is just a small example extracted from the benchmark to reproduce the issue. The main problem is that in the benchmark, after the core loop is unrolled, the rdcycle instruction is moved to the middle of the workload, causing a significant deviation in the final calculated cycle count. Moreover, the code in the benchmark can't be fixed in this simple way. The impact of this issue is that the benchmark scores obtained using the clang compiler become unreliable, so we still hope to maintain consistency with the behavior of gcc.

[topperc]

I don't think inline assembly makes any guarantees for what you're trying to do here. You can fix it by passing res as input to the second rdcycle asm block. https://godbolt.org/z/7GeE9o5Ev That will guarantee it is computed before the asm block.
This description from gcc's documentation is relevant. https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile-1

Note that the compiler can move even volatile asm instructions relative to other code, including across jump instructions. For example, on many targets there is a system register that controls the rounding mode of floating-point operations. Setting it with a volatile asm statement, as in the following PowerPC example, does not work reliably.

asm volatile("mtfsf 255, %0" : : "f" (fpenv));
sum = x + y;
The compiler may move the addition back before the volatile asm statement. To make it work as expected, add an artificial dependency to the asm by referencing a variable in the subsequent code, for example:

asm volatile ("mtfsf 255,%1" : "=X" (sum) : "f" (fpenv));
sum = x + y;

This is just a small example extracted from the benchmark to reproduce the issue. The main problem is that in the benchmark, after the core loop is unrolled, the rdcycle instruction is moved to the middle of the workload, causing a significant deviation in the final calculated cycle count. Moreover, the code in the benchmark can't be fixed in this simple way. The impact of this issue is that the benchmark scores obtained using the clang compiler become unreliable, so we still hope to maintain consistency with the behavior of gcc.

The scheduler in LLVM builds a dependency graph using the inputs and outputs of all instructions. For this purpose the inline assembly block is treated as single instruction with the inputs and outputs used for dependencies. Artificial edges are added to the graph to main ordering between loads, stores, and volatile inline assembly.

Scheduling uses this graph to enforce ordering that shouldn't break. Because the inline assembly doesn't depend on the adds there is no relationship between them in the dependency graph.

I'm looking to see if I can find a way to fix this which isn't just make all inline assembly blocks scheduling boundaries.

[fanghuaqi]

same issue also present in gcc 15 trunk version, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119280

case here https://godbolt.org/z/T9zGhhqGT