[MIPS] __builtin_bswap16 miscompilation #103035

markus-oberhumer · 2024-08-13T10:05:14Z

(EDIT: I've added a new Godbolt link and updated the example to make it clear that the bug needs -march=mips32 to trigger)

Godbolt link: https://gcc.godbolt.org/z/vMGc86deh

clang 18.1.8 MIPS miscompilation in Debug mode

// clang -target mips-linux-musl -march=mips32 bug.c -o bug.out
// qemu-mips ./bug.out

unsigned bswap16_fails(unsigned v) {
    // FAILS - cast seems ignored ??
    return __builtin_bswap16((unsigned short) v);
}

unsigned bswap16_ok(unsigned v) {
    return __builtin_bswap16((unsigned short) (v & 0xffff)); // OK
}

int main(void) {
    if (bswap16_ok(0x04030201) != 0x0102)
        return 1;
    if (bswap16_fails(0x04030201) != 0x0102)
        return 2; // ERROR HERE
    return 0;
}

The text was updated successfully, but these errors were encountered:

llvmbot · 2024-08-13T10:12:59Z

@llvm/issue-subscribers-backend-mips

Author: Markus F.X.J. Oberhumer (markus-oberhumer)

clang 18.1.8 MIPS miscompilation in Debug mode

// clang -target mips-linux-musl bug.c -o bug.out
// qemu-mips ./bug.out

unsigned bswap16_fails(unsigned v) {
    // FAILS - cast seems ignored ??
    return __builtin_bswap16((unsigned short) v);
}

unsigned bswap16_ok(unsigned v) {
    return __builtin_bswap16((unsigned short) (v &amp; 0xffff)); // OK
}

int main() {
    if (bswap16_ok(0x04030201) != 0x0102)
        return 1;
    if (bswap16_fails(0x04030201) != 0x0102)
        return 2; // ERROR HERE
}

dtcxzyw · 2024-08-13T14:23:13Z

https://godbolt.org/z/MGWGPz1aE

bswap16_fails:                          # @bswap16_fails
        addiu   $sp, $sp, -8
        sw      $4, 4($sp)
        sll     $1, $4, 8
        andi    $2, $4, 65280
        srl     $2, $2, 8
        or      $1, $1, $2
        andi    $2, $1, 65535
        jr      $ra
        addiu   $sp, $sp, 8
bswap16_ok:                             # @bswap16_ok
        addiu   $sp, $sp, -8
        sw      $4, 4($sp)
        sll     $1, $4, 8
        andi    $2, $4, 65280
        srl     $2, $2, 8
        or      $1, $1, $2
        andi    $2, $1, 65535
        jr      $ra
        addiu   $sp, $sp, 8
main:                                   # @main
        addiu   $sp, $sp, -32
        sw      $ra, 28($sp)                    # 4-byte Folded Spill
        sw      $17, 24($sp)                    # 4-byte Folded Spill
        sw      $16, 20($sp)                    # 4-byte Folded Spill
        sw      $zero, 16($sp)
        lui     $1, 1027
        ori     $16, $1, 513
        jal     bswap16_ok
        move    $4, $16
        addiu   $17, $zero, 258
        beq     $2, $17, $BB2_2
        nop
        addiu   $1, $zero, 1
        j       $BB2_4
        sw      $1, 16($sp)
$BB2_2:                                 # %if.end
        jal     bswap16_fails
        move    $4, $16
        beq     $2, $17, $BB2_4
        nop
        addiu   $1, $zero, 2
        sw      $1, 16($sp)
$BB2_4:                                 # %if.end4
        lw      $2, 16($sp)
        lw      $16, 20($sp)                    # 4-byte Folded Reload
        lw      $17, 24($sp)                    # 4-byte Folded Reload
        lw      $ra, 28($sp)                    # 4-byte Folded Reload
        jr      $ra
        addiu   $sp, $sp, 32

I cannot reproduce this issue.

markus-oberhumer · 2024-08-13T17:12:40Z

@dtcxzyw Please try this link https://gcc.godbolt.org/z/cseeoKe35

This bug might be a clang/frontend issue.

(Edit: note that the bug only happens when using NO optimizations)

dtcxzyw · 2024-08-13T17:41:49Z

bswap16_fails(unsigned int):
        addiu   $sp, $sp, -16
        sw      $ra, 12($sp)
        sw      $fp, 8($sp)
        move    $fp, $sp
        sw      $4, 4($fp)
        lw      $1, 4($fp)
        wsbh    $1, $1
        andi    $2, $1, 65535
        move    $sp, $fp
        lw      $fp, 8($sp)
        lw      $ra, 12($sp)
        addiu   $sp, $sp, 16
        jr      $ra
        nop

bswap16_ok(unsigned int):
        addiu   $sp, $sp, -16
        sw      $ra, 12($sp)
        sw      $fp, 8($sp)
        move    $fp, $sp
        sw      $4, 4($fp)
        lw      $1, 4($fp)
        ori     $2, $zero, 65535
        and     $1, $1, $2
        wsbh    $1, $1
        andi    $2, $1, 65535
        move    $sp, $fp
        lw      $fp, 8($sp)
        lw      $ra, 12($sp)
        addiu   $sp, $sp, 16
        jr      $ra
        nop

These two pieces of assembly code look equivalent.
Can you print out the result of bswap16_fails(0x04030201)?

cc @topperc

markus-oberhumer · 2024-08-13T17:49:58Z

I get

bswap16_fails(0x04030201) == 0x00000302 WRONG

bswap16_ok(0x04030201) == 0x00000102 OK

(Edit: using qemu-mips)

topperc · 2024-08-13T18:50:34Z

Best guess is it's a problem with fast-isel. Can you try with -mllvm -fast-isel=false

markus-oberhumer · 2024-08-13T18:53:46Z

@topperc Indeed, -mllvm -fast-isel=false fixes the problem!

We need to mask the SRL result to 8 bits before ORing in the SLL. This is needed in case bits 23:16 of the input aren't zero. They will have been shifted into bits 15:8. We don't need to AND the result with 0xffff. It's ok if the upper 16 bits of the register is garbage. Fixes llvm#103035.

topperc · 2024-08-13T20:24:50Z

Can you check what you get for bswap16_fails(0x14233241)?

markus-oberhumer · 2024-08-14T09:47:39Z

bswap16_fails(0x14233241) gives 0x00006332 for me.

Are there any command line flags I could testing enabling/disabling hasMips32r2 ?

topperc · 2024-08-14T14:08:18Z

bswap16_fails(0x14233241) gives 0x00006332 for me.

So it does appear that byte 0 and byte 2 are ORed together to form byte 1 or the result. But the godbolt link doesn't show any OR instructions https://gcc.godbolt.org/z/cseeoKe35 Can you confirm with objdump if you are getting the same assembly as godbolt?

markus-oberhumer · 2024-08-14T14:26:40Z

Ok, here is what I get - it looks like zig cc [1] defaults differ from Godbolt clang defaults. Maybe some -march setting - how can I enable/disable hasMips32r2?

$ zig cc -target mips-linux-musl -c bug.c
$ llvm-objdump -d bug.o

bug.o:	file format elf32-mips

Disassembly of section .text:

00000000 <bswap16_fails>:
       0: 27 bd ff f0  	addiu	$sp, $sp, -0x10 <bswap16_ok+0xffffffffffffffac>
       4: af bf 00 0c  	sw	$ra, 0xc($sp)
       8: af be 00 08  	sw	$fp, 0x8($sp)
       c: 03 a0 f0 25  	move	$fp, $sp
      10: af c4 00 04  	sw	$4, 0x4($fp)
      14: 8f c2 00 04  	lw	$2, 0x4($fp)
      18: 00 02 0a 00  	sll	$1, $2, 0x8 <bswap16_fails+0x8>
      1c: 00 02 12 02  	srl	$2, $2, 0x8 <bswap16_fails+0x8>
      20: 00 22 08 25  	or	$1, $1, $2
      24: 30 21 ff ff  	andi	$1, $1, 0xffff <bswap16_ok+0xffbb>
      28: 30 22 ff ff  	andi	$2, $1, 0xffff <bswap16_ok+0xffbb>
      2c: 03 c0 e8 25  	move	$sp, $fp
      30: 8f be 00 08  	lw	$fp, 0x8($sp)
      34: 8f bf 00 0c  	lw	$ra, 0xc($sp)
      38: 27 bd 00 10  	addiu	$sp, $sp, 0x10 <bswap16_fails+0x10>
      3c: 03 e0 00 08  	jr	$ra
      40: 00 00 00 00  	nop <bswap16_fails>

00000044 <bswap16_ok>:
      44: 27 bd ff f0  	addiu	$sp, $sp, -0x10 <bswap16_ok+0xffffffffffffffac>
      48: af bf 00 0c  	sw	$ra, 0xc($sp)
      4c: af be 00 08  	sw	$fp, 0x8($sp)
      50: 03 a0 f0 25  	move	$fp, $sp
      54: af c4 00 04  	sw	$4, 0x4($fp)
      58: 8f c1 00 04  	lw	$1, 0x4($fp)
      5c: 34 02 ff ff  	ori	$2, $zero, 0xffff <bswap16_ok+0xffbb>
      60: 00 22 10 24  	and	$2, $1, $2
      64: 00 02 0a 00  	sll	$1, $2, 0x8 <bswap16_fails+0x8>
      68: 00 02 12 02  	srl	$2, $2, 0x8 <bswap16_fails+0x8>
      6c: 00 22 08 25  	or	$1, $1, $2
      70: 30 21 ff ff  	andi	$1, $1, 0xffff <bswap16_ok+0xffbb>
      74: 30 22 ff ff  	andi	$2, $1, 0xffff <bswap16_ok+0xffbb>
      78: 03 c0 e8 25  	move	$sp, $fp
      7c: 8f be 00 08  	lw	$fp, 0x8($sp)
      80: 8f bf 00 0c  	lw	$ra, 0xc($sp)
      84: 27 bd 00 10  	addiu	$sp, $sp, 0x10 <bswap16_fails+0x10>
      88: 03 e0 00 08  	jr	$ra
      8c: 00 00 00 00  	nop <bswap16_fails>

[1] https://ziglang.org/download/

markus-oberhumer · 2024-08-14T14:38:18Z

Update: just found the obvious -march=mips32r2 option, and testing seems fine with that switch.

So your patch at #103398 looks good.

P.S: Sorry for the confusion, I'm no MIPS expert.

We need to mask the SRL result to 8 bits before ORing in the SLL. This is needed in case bits 23:16 of the input aren't zero. They will have been shifted into bits 15:8. We don't need to AND the result with 0xffff. It's ok if the upper 16 bits of the register are garbage. Fixes #103035.

nikic · 2024-08-19T08:11:39Z

/cherry-pick ebe7265

We need to mask the SRL result to 8 bits before ORing in the SLL. This is needed in case bits 23:16 of the input aren't zero. They will have been shifted into bits 15:8. We don't need to AND the result with 0xffff. It's ok if the upper 16 bits of the register are garbage. Fixes llvm#103035. (cherry picked from commit ebe7265)

llvmbot · 2024-08-19T08:16:43Z

/pull-request #104745

We need to mask the SRL result to 8 bits before ORing in the SLL. This is needed in case bits 23:16 of the input aren't zero. They will have been shifted into bits 15:8. We don't need to AND the result with 0xffff. It's ok if the upper 16 bits of the register are garbage. Fixes llvm#103035. (cherry picked from commit ebe7265)

github-actions bot added the new issue label Aug 13, 2024

Sirraide added backend:MIPS and removed new issue labels Aug 13, 2024

EugeneZelenko added the miscompilation label Aug 13, 2024

topperc self-assigned this Aug 13, 2024

topperc mentioned this issue Aug 13, 2024

[Mips] Fix fast isel for i16 bswap. #103398

Merged

topperc closed this as completed in #103398 Aug 16, 2024

nikic added this to the LLVM 19.X Release milestone Aug 19, 2024

github-project-automation bot added this to LLVM Release Status Aug 19, 2024

github-project-automation bot moved this to Needs Triage in LLVM Release Status Aug 19, 2024

nikic moved this from Needs Triage to Done in LLVM Release Status Aug 19, 2024

EugeneZelenko added the release:backport label Aug 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIPS] __builtin_bswap16 miscompilation #103035

[MIPS] __builtin_bswap16 miscompilation #103035

markus-oberhumer commented Aug 13, 2024 •

edited

Loading

llvmbot commented Aug 13, 2024

dtcxzyw commented Aug 13, 2024

markus-oberhumer commented Aug 13, 2024 •

edited

Loading

dtcxzyw commented Aug 13, 2024

markus-oberhumer commented Aug 13, 2024 •

edited

Loading

topperc commented Aug 13, 2024

markus-oberhumer commented Aug 13, 2024

topperc commented Aug 13, 2024

markus-oberhumer commented Aug 14, 2024

topperc commented Aug 14, 2024

markus-oberhumer commented Aug 14, 2024

markus-oberhumer commented Aug 14, 2024

nikic commented Aug 19, 2024

llvmbot commented Aug 19, 2024

[MIPS] __builtin_bswap16 miscompilation #103035

[MIPS] __builtin_bswap16 miscompilation #103035

Comments

markus-oberhumer commented Aug 13, 2024 • edited Loading

llvmbot commented Aug 13, 2024

dtcxzyw commented Aug 13, 2024

markus-oberhumer commented Aug 13, 2024 • edited Loading

dtcxzyw commented Aug 13, 2024

markus-oberhumer commented Aug 13, 2024 • edited Loading

topperc commented Aug 13, 2024

markus-oberhumer commented Aug 13, 2024

topperc commented Aug 13, 2024

markus-oberhumer commented Aug 14, 2024

topperc commented Aug 14, 2024

markus-oberhumer commented Aug 14, 2024

markus-oberhumer commented Aug 14, 2024

nikic commented Aug 19, 2024

llvmbot commented Aug 19, 2024

markus-oberhumer commented Aug 13, 2024 •

edited

Loading

markus-oberhumer commented Aug 13, 2024 •

edited

Loading

markus-oberhumer commented Aug 13, 2024 •

edited

Loading