[llvm] [Mips] Fix missing sign extension in expansion of sub-word atomic max (PR #77072)

via llvm-commits llvm-commits at lists.llvm.org
Sun Apr 14 18:20:08 PDT 2024


yingopq wrote:

> Clarify: I don't have any knowledge about MIPS, and the following is only confirmed from the output logs. Please correct me if there is any mistake. :)
> 
> TL;DR: All issues have been fixed in LLVM 17. This appears to be an incorrect patch; we might consider reverting it. But I hope @yingopq and others can come to confirm this.
> 
> Consider the following two pieces of code:
> 
> ```llvm
> ; v1.ll https://github.com/rust-lang/rust/issues/100650
> define i8 @test(ptr %arg) {
>   %i6 = atomicrmw max ptr %arg, i8 -25 seq_cst, align 1
>   %i7 = load atomic i8, ptr %arg seq_cst, align 1
>   ret i8 %i7
> }
> 
> define i32 @main() {
>   %i = alloca i8, align 1
>   store i8 30, ptr %i, align 1
>   %1 = call i8 @test(ptr %i)
>   %2 = sext i8 %1 to i32
>   ret i32 %2
> }
> ```
> 
> and
> 
> ```llvm
> ; v2.ll https://github.com/rust-lang/rust/issues/123772
> define i8 @test(ptr %arg) {
>   %i6 = atomicrmw max ptr %arg, i8 42 seq_cst, align 1
>   ret i8 %i6
> }
> 
> define i32 @main() {
>   %i = alloca i8, align 1
>   store i8 23, ptr %i, align 1
>   %1 = call signext i8 @test(ptr %i)
>   %2 = sext i8 %1 to i32
>   ret i32 %2
> }
> ```
> 
> I can confirm that the return value of `v2.ll` from llvmorg-17-init to 18.1.2 is always 23. However, starting from 18.1.3, it changes to 0. For `v1.ll`, starting with the commit [c65b4d6](https://github.com/llvm/llvm-project/commit/c65b4d64d4b09795fe237b62a4226121c5b13248), the return value changes from 231 to 30. However, starting from 18.1.3, it also changes to 0.
> 
> This is my verification script:
> 
> ```shell
> LLC=path/llc
> MIPSEL_GCC=path/mipsel-unknown-linux-gnu-gcc
> 
> $LLC -march=mipsel -O0 -mcpu=mips32 -verify-machineinstrs v1.ll -filetype=obj -o v1.o
> $MIPSEL_GCC -o v1 v1.o
> qemu-mipsel v1
> mipsel=$?
> $LLC -O0 -march=x86-64 v1.ll -filetype=obj -o v1.o
> clang -o v1 v1.o
> ./v1
> x86=$?
> echo "v1: $mipsel == $x86"
> 
> $LLC -march=mipsel -O0 -mcpu=mips32 -verify-machineinstrs v2.ll -filetype=obj -o v2.o
> $MIPSEL_GCC -o v2 v2.o
> qemu-mipsel v2
> mipsel=$?
> $LLC -O0 -march=x86-64 v2.ll -filetype=obj -o v2.o
> clang -o v2 v2.o
> ./v2
> x86=$?
> if [ "$mipsel" != "$x86" ]; then
>   bad=$((bad + 1))
> fi
> echo "v2: $mipsel == $x86"
> ```

Thanks for your detail comment.
The reason about submitting this patch is that code did not do correct sign extension.  And when I verify this patch, I use ``` %i = alloca i32, align 4 ```, so did not find the current issue you found.  And I am investigating.

https://github.com/llvm/llvm-project/pull/77072


More information about the llvm-commits mailing list