Bad codegen for reversing bits in bytes without swapping bytes

[tom-rein]

The following code on RISC-V target with Zbkb extension (godbolt):

#include <cstdint>
uint8_t brev8(uint8_t x) {
    return __builtin_bitreverse8(x);
}

Results in:

brev8(unsigned char):
        rev8    a0, a0
        brev8   a0, a0
        srli    a0, a0, 56
        ret

The expected result would be just brev8 a0, a0 since it doesn't swap bytes.

On armv8 target (and similarly on armv7) bit reversing bytes in uint16_t uses rev16 which results in an extra shift:

uint16_t brev8(uint16_t x) {
    return __builtin_bswap16(__builtin_bitreverse16(x));
}

brev8(unsigned short):
        rbit    w8, w0
        lsr     w8, w8, #16
        rev16   w0, w8
        ret

This could instead be:

brev8(unsigned short):
        rbit    w0, w0
        rev     w0, w0
        ret

(Also the register allocation somehow uses w8 instead of reusing w0, which I find weird, since that is done on other targets, including armv7)

When doing bswap16 first and then bitreverse16, it doesn't even use rev16, but adds two shifts:

uint16_t brev8_2(uint16_t x) {
    return __builtin_bitreverse16(__builtin_bswap16(x));
}

brev8_2(unsigned short):
        rev     w8, w0
        lsr     w8, w8, #16
        rbit    w8, w8
        lsr     w0, w8, #16
        ret

[llvmbot]

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

Author: Tom Reinhardt (tom-rein)

The following code on RISC-V target with Zbkb extension ([godbolt](https://godbolt.org/z/cTqEYsbaz)): ```C++ #include <cstdint> uint8_t brev8(uint8_t x) { return __builtin_bitreverse8(x); } ``` Results in: ``` brev8(unsigned char): rev8 a0, a0 brev8 a0, a0 srli a0, a0, 56 ret ``` The expected result would be just `brev8 a0, a0` since it doesn't swap bytes.

On armv8 target (and similarly on armv7) bit reversing bytes in uint16_t uses rev16 which results in an extra shift:

uint16_t brev8(uint16_t x) {
    return __builtin_bswap16(__builtin_bitreverse16(x));
}

brev8(unsigned short):
        rbit    w8, w0
        lsr     w8, w8, #<!-- -->16
        rev16   w0, w8
        ret

This could instead be:

brev8(unsigned short):
        rbit    w0, w0
        rev     w0, w0
        ret

(Also the register allocation somehow uses w8 instead of reusing w0, which I find weird, since that is done on other targets, including armv7)

When doing bswap16 first and then bitreverse16, it doesn't even use rev16, but adds two shifts:

uint16_t brev8_2(uint16_t x) {
    return __builtin_bitreverse16(__builtin_bswap16(x));
}

brev8_2(unsigned short):
        rev     w8, w0
        lsr     w8, w8, #<!-- -->16
        rbit    w8, w8
        lsr     w0, w8, #<!-- -->16
        ret

[topperc]

@tom-rein are you reporting 2 issues? One for RISC-V and one for ARM/AArch64?

[tom-rein]

@topperc yes

[llvmbot]

@llvm/issue-subscribers-backend-arm

Author: Tom Reinhardt (tom-rein)

The following code on RISC-V target with Zbkb extension ([godbolt](https://godbolt.org/z/cTqEYsbaz)): ```C++ #include <cstdint> uint8_t brev8(uint8_t x) { return __builtin_bitreverse8(x); } ``` Results in: ``` brev8(unsigned char): rev8 a0, a0 brev8 a0, a0 srli a0, a0, 56 ret ``` The expected result would be just `brev8 a0, a0` since it doesn't swap bytes.

On armv8 target (and similarly on armv7) bit reversing bytes in uint16_t uses rev16 which results in an extra shift:

uint16_t brev8(uint16_t x) {
    return __builtin_bswap16(__builtin_bitreverse16(x));
}

brev8(unsigned short):
        rbit    w8, w0
        lsr     w8, w8, #<!-- -->16
        rev16   w0, w8
        ret

This could instead be:

brev8(unsigned short):
        rbit    w0, w0
        rev     w0, w0
        ret

(Also the register allocation somehow uses w8 instead of reusing w0, which I find weird, since that is done on other targets, including armv7)

When doing bswap16 first and then bitreverse16, it doesn't even use rev16, but adds two shifts:

uint16_t brev8_2(uint16_t x) {
    return __builtin_bitreverse16(__builtin_bswap16(x));
}

brev8_2(unsigned short):
        rev     w8, w0
        lsr     w8, w8, #<!-- -->16
        rbit    w8, w8
        lsr     w0, w8, #<!-- -->16
        ret

[llvmbot]

@llvm/issue-subscribers-backend-aarch64

Author: Tom Reinhardt (tom-rein)

The following code on RISC-V target with Zbkb extension ([godbolt](https://godbolt.org/z/cTqEYsbaz)): ```C++ #include <cstdint> uint8_t brev8(uint8_t x) { return __builtin_bitreverse8(x); } ``` Results in: ``` brev8(unsigned char): rev8 a0, a0 brev8 a0, a0 srli a0, a0, 56 ret ``` The expected result would be just `brev8 a0, a0` since it doesn't swap bytes.

On armv8 target (and similarly on armv7) bit reversing bytes in uint16_t uses rev16 which results in an extra shift:

uint16_t brev8(uint16_t x) {
    return __builtin_bswap16(__builtin_bitreverse16(x));
}

brev8(unsigned short):
        rbit    w8, w0
        lsr     w8, w8, #<!-- -->16
        rev16   w0, w8
        ret

This could instead be:

brev8(unsigned short):
        rbit    w0, w0
        rev     w0, w0
        ret

(Also the register allocation somehow uses w8 instead of reusing w0, which I find weird, since that is done on other targets, including armv7)

When doing bswap16 first and then bitreverse16, it doesn't even use rev16, but adds two shifts:

uint16_t brev8_2(uint16_t x) {
    return __builtin_bitreverse16(__builtin_bswap16(x));
}

brev8_2(unsigned short):
        rev     w8, w0
        lsr     w8, w8, #<!-- -->16
        rbit    w8, w8
        lsr     w0, w8, #<!-- -->16
        ret

[topperc]

The sext.w in a couple of those RISC-V tests is also unnecessary.

Scratch that. That's not true.