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

+; MIPS32-NEXT:  $BB4_1: # %entry
+; MIPS32-NEXT:    # =>This Inner Loop Header: Depth=1
+; MIPS32-NEXT:    ll $2, 0($6)
+; MIPS32-NEXT:    and $2, $2, $8


Are these and instructions needed? The srav will move the 16 bit chunk to the LSBs, then the sll+sra pair will fill the upper 16 bits with the sign of the lower 16 bits. Doesn't seem like anything needs ANDed before hand.

Add sign extension "SEB/SEH" before compare. Fix llvm#61881

brad0 · 2024-03-08T20:26:21Z

@topperc

topperc

LGTM

…llvm#77072) Add sign extension "SEB/SEH" before compare. Fix llvm#61881 (cherry picked from commit 755b439)

martn3 · 2024-04-11T06:31:32Z

Hello, I started to see MIPS failures in the Rust standard library tests after this change. For example, this program begins to fail after this change:

fn main() {
    let x = std::sync::atomic::AtomicI8::new(23);
    assert_eq!(x.fetch_max(42, std::sync::atomic::Ordering::SeqCst), 23);
}

Please see rust-lang/rust#123772 for more details.

This seems quite problematic, so please let me know if I can help investigate this somehow.

yingopq · 2024-04-11T07:05:56Z

Hello, I started to see MIPS failures in the Rust standard library tests after this change. For example, this program begins to fail after this change:
fn main() {
    let x = std::sync::atomic::AtomicI8::new(23);
    assert_eq!(x.fetch_max(42, std::sync::atomic::Ordering::SeqCst), 23);
}
Please see rust-lang/rust#123772 for more details.

This seems quite problematic, so please let me know if I can help investigate this somehow.

hi, please use code 0e501db to test again, because we submit another patch after fbb27d1. This issue include two patch.
Thanks, please let me know if you have any other questions.

nikic · 2024-04-11T08:23:35Z

@yingopq As I understand it, this was reproduced with the release/18.x branch (or rust's fork, which is essentially the same), which already contains that second commit as well.

yingopq · 2024-04-11T08:29:09Z

@yingopq As I understand it, this was reproduced with the release/18.x branch (or rust's fork, which is essentially the same), which already contains that second commit as well.

In rust-lang/rust#123772, he comments With src/llvm-project pointing to https://github.com/llvm/llvm-project/commit/fbb27d16fa12aa595cbd20a1fb5f1c5b80748fa4 the test also fails, but when pointing src/llvm-project to the parent commit (https://github.com/llvm/llvm-project/commit/e74c1678231ab2a7c6defe8bb90366e664780d6f) the test passes.. So I reply him use code 0e501db

martn3 · 2024-04-11T08:36:19Z

Hi @yingopq, thank you for the reply and for the tip. I double-checked, and I get the test failure also with 0e501db. I also tested some extra commits just to be sure. Here are my results:

Commit	Commit info	Commit Title	Test result
`dc39028`	branch main	[mlir] Fix -Wsign-compare in ValueBoundsOpInterface.cpp (NFC)	Fail
`b6ebea7`	branch release/18.x	[SPARC] Implement L and H inline asm argument modifiers	Fail
`7c7c3d4`	tag llvmorg-18.1.2 + 13	[ODS][NFC] Cast range.size() to int32_t in accumulation	Fail
`0e501db`	tag llvmorg-18.1.2 + 12	[Mips] Restore wrong deletion of instruction 'and' in unsigned min/max processing.	Fail
`fbb27d1`	tag llvmorg-18.1.2 + 11	[Mips] Fix missing sign extension in expansion of sub-word atomic max	Fail
`e74c167`	tag llvmorg-18.1.2 + 10	[PowerPC] provide CFI for ELF32 to unwind cr2, cr3, cr4	OK
`1c7c16e`	tag llvmorg-18.1.2 + 9	[NFC][PowerPC] use script to regenerate the CHECK lines	OK

Please let me know if you would like me to perform more tests.

yingopq · 2024-04-11T08:42:29Z

Hi @yingopq, thank you for the reply and for the tip. I double-checked, and I get the test failure also with 0e501db. I also tested some extra commits just to be sure. Here are my results:

Commit Commit info Commit Title Test result
dc39028 branch main [mlir] Fix -Wsign-compare in ValueBoundsOpInterface.cpp (NFC) Fail
b6ebea7 branch release/18.x [SPARC] Implement L and H inline asm argument modifiers Fail
7c7c3d4 tag llvmorg-18.1.2 + 13 [ODS][NFC] Cast range.size() to int32_t in accumulation Fail
0e501db tag llvmorg-18.1.2 + 12 [Mips] Restore wrong deletion of instruction 'and' in unsigned min/max processing. Fail
fbb27d1 tag llvmorg-18.1.2 + 11 [Mips] Fix missing sign extension in expansion of sub-word atomic max Fail
e74c167 tag llvmorg-18.1.2 + 10 [PowerPC] provide CFI for ELF32 to unwind cr2, cr3, cr4 OK
1c7c16e tag llvmorg-18.1.2 + 9 [NFC][PowerPC] use script to regenerate the CHECK lines OK
Please let me know if you would like me to perform more tests.

Thanks, I would check it.

DianQK · 2024-04-12T05:56:00Z

I have reduced to the following code:

target datalayout = "e-m:m-p:32:32-i8:8:32-i16:16:32-i64:64-n32-S64"
target triple = "mipsel-unknown-linux-gnu"

define i32 @main() {
  %1 = call signext i8 @test()
  %2 = sext i8 %1 to i32
  ret i32 %2
}

define signext i8 @test() {
  %i = alloca i8, align 1
  store i8 23, ptr %i, align 1
  %i1 = atomicrmw max ptr %i, i8 42 seq_cst, align 1
  ret i8 %i1
}

https://llvm.godbolt.org/z/4Mxnc1Eda

@yingopq I hope this can be of some use.

There might be no need to create a variable:

define i8 @test(ptr %arg) {
  %i6 = atomicrmw max ptr %arg, i8 42 seq_cst, align 1
  ret i8 %i6
}

DianQK · 2024-04-13T04:19:40Z

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:

; 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

; 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, the return value changes from 231 to 30. However, starting from 18.1.3, it also changes to 0.

This is my verification script:

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"

yingopq · 2024-04-15T01:20:01Z

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:

; 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

; 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, the return value changes from 231 to 30. However, starting from 18.1.3, it also changes to 0.

This is my verification script:

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.

DianQK · 2024-04-15T10:05:19Z

Thank you very much for looking into it. Would you mind temporarily reverting these two commits? :>

…omic max (llvm#77072)" These changes caused correctness regressions observed in Rust, see llvm#77072 (comment). This reverts commit 0e501db. This reverts commit fbb27d1.

topperc · 2024-04-16T01:30:40Z

There's definitely an issue in this sequence

; MIPSEL-NEXT:    srav $2, $2, $10
; MIPSEL-NEXT:    srav $7, $7, $10
; MIPSEL-NEXT:    seh $2, $2
; MIPSEL-NEXT:    seh $7, $7
; MIPSEL-NEXT:    slt $5, $2, $7
; MIPSEL-NEXT:    move $3, $2
; MIPSEL-NEXT:    movn $3, $7, $5

The move and movn need to use the values in $2 and $7 before the srav and seh. The rest of the code expect the 16 bit value to be in the same place in the register as it was when it came out of the load.

…omic max (llvm#77072)" These changes caused correctness regressions observed in Rust, see llvm#77072 (comment). This reverts commit 0e501db. This reverts commit fbb27d1.

yingopq · 2024-04-17T07:04:32Z

There's definitely an issue in this sequence
; MIPSEL-NEXT:    srav $2, $2, $10
; MIPSEL-NEXT:    srav $7, $7, $10
; MIPSEL-NEXT:    seh $2, $2
; MIPSEL-NEXT:    seh $7, $7
; MIPSEL-NEXT:    slt $5, $2, $7
; MIPSEL-NEXT:    move $3, $2
; MIPSEL-NEXT:    movn $3, $7, $5
The move and movn need to use the values in $2 and $7 before the srav and seh. The rest of the code expect the 16 bit value to be in the same place in the register as it was when it came out of the load.

@topperc Thanks so much for pointing out the key to the problem, and I have verified that this is indeed the problem. I would submit patch again.

brad0 requested review from jyknight and topperc January 6, 2024 08:48

yingopq force-pushed the Fix_bug_61881 branch 2 times, most recently from 0cdecec to d96c168 Compare January 8, 2024 02:39

brad0 requested a review from MaskRay January 20, 2024 05:04

topperc reviewed Feb 9, 2024

View reviewed changes

llvm/lib/Target/Mips/MipsExpandPseudo.cpp Show resolved Hide resolved

yingopq force-pushed the Fix_bug_61881 branch from 0d2741e to c8b484f Compare February 21, 2024 11:03

brad0 requested a review from topperc February 21, 2024 21:38

topperc reviewed Feb 27, 2024

View reviewed changes

[Mips] Fix missing sign extension in expansion of sub-word atomic max

94268c6

Add sign extension "SEB/SEH" before compare. Fix llvm#61881

yingopq force-pushed the Fix_bug_61881 branch from c8b484f to 94268c6 Compare March 4, 2024 06:31

brad0 requested a review from topperc March 4, 2024 06:33

topperc approved these changes Mar 8, 2024

View reviewed changes

brad0 merged commit 755b439 into llvm:main Mar 8, 2024
4 checks passed

brad0 mentioned this pull request Mar 8, 2024

Merge 755b439694432d4f68e20e979b479cbc30602bb1 into 18.x #84564

Closed

Amanieu mentioned this pull request Mar 8, 2024

fetch_max broken for atomici8 and atomici16 on some targets. rust-lang/rust#100650

Open

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 8, 2024

[Mips] Fix missing sign extension in expansion of sub-word atomic max (…

86b5745

…llvm#77072) Add sign extension "SEB/SEH" before compare. Fix llvm#61881 (cherry picked from commit 755b439)

brad0 mentioned this pull request Mar 24, 2024

Merge 755b439694432d4f68e20e979b479cbc30602bb1 into 18.1.x #86423

Closed

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 24, 2024

[Mips] Fix missing sign extension in expansion of sub-word atomic max (…

2713099

…llvm#77072) Add sign extension "SEB/SEH" before compare. Fix llvm#61881 (cherry picked from commit 755b439)

tstellar pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 27, 2024

[Mips] Fix missing sign extension in expansion of sub-word atomic max (…

fbb27d1

…llvm#77072) Add sign extension "SEB/SEH" before compare. Fix llvm#61881 (cherry picked from commit 755b439)

martn3 mentioned this pull request Apr 11, 2024

[MIPS] AtomicI8 and AtomicI16 doctests for fn fetch_min() and fn fetch_max() began failing in nightly-2024-04-08 rust-lang/rust#123772

Closed

nikic mentioned this pull request Apr 16, 2024

Revert "[Mips] Fix missing sign extension in expansion of sub-word atomic max (#77072)" #88818

Merged

jdmitrovic-syrmia mentioned this pull request Apr 18, 2024

[MIPS] Sign-extend subwords when expanding atomic max/min #89246

Closed

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yingopq commented Jan 5, 2024

github-actions bot commented Jan 5, 2024 •

edited

Loading

yingopq commented Jan 8, 2024

brad0 commented Jan 14, 2024

yingopq commented Jan 23, 2024

yingopq commented Feb 4, 2024

brad0 commented Feb 8, 2024

brad0 commented Feb 14, 2024

brad0 commented Feb 24, 2024

brad0 commented Feb 27, 2024

topperc Feb 27, 2024

brad0 commented Mar 8, 2024

topperc left a comment

martn3 commented Apr 11, 2024

yingopq commented Apr 11, 2024

nikic commented Apr 11, 2024

yingopq commented Apr 11, 2024

martn3 commented Apr 11, 2024

yingopq commented Apr 11, 2024

DianQK commented Apr 12, 2024 •

edited

Loading

DianQK commented Apr 13, 2024

yingopq commented Apr 15, 2024

DianQK commented Apr 15, 2024

topperc commented Apr 16, 2024

yingopq commented Apr 17, 2024

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

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

Conversation

yingopq commented Jan 5, 2024

github-actions bot commented Jan 5, 2024 • edited Loading

yingopq commented Jan 8, 2024

brad0 commented Jan 14, 2024

yingopq commented Jan 23, 2024

yingopq commented Feb 4, 2024

brad0 commented Feb 8, 2024

brad0 commented Feb 14, 2024

brad0 commented Feb 24, 2024

brad0 commented Feb 27, 2024

topperc Feb 27, 2024

Choose a reason for hiding this comment

brad0 commented Mar 8, 2024

topperc left a comment

Choose a reason for hiding this comment

martn3 commented Apr 11, 2024

yingopq commented Apr 11, 2024

nikic commented Apr 11, 2024

yingopq commented Apr 11, 2024

martn3 commented Apr 11, 2024

yingopq commented Apr 11, 2024

DianQK commented Apr 12, 2024 • edited Loading

DianQK commented Apr 13, 2024

yingopq commented Apr 15, 2024

DianQK commented Apr 15, 2024

topperc commented Apr 16, 2024

yingopq commented Apr 17, 2024

github-actions bot commented Jan 5, 2024 •

edited

Loading

DianQK commented Apr 12, 2024 •

edited

Loading