Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MIPS] Incorrect expansion of sub-word signed atomic max #61881

Closed
Amanieu opened this issue Apr 1, 2023 · 4 comments · Fixed by #77072 or #85902
Closed

[MIPS] Incorrect expansion of sub-word signed atomic max #61881

Amanieu opened this issue Apr 1, 2023 · 4 comments · Fixed by #77072 or #85902

Comments

@Amanieu
Copy link
Contributor

Amanieu commented Apr 1, 2023

IR

define void @foo(ptr %a, i32 %x) {
  %val = trunc i32 %x to i8
  %1 = atomicrmw max ptr %a, i8 %val seq_cst
  ret void
}

Asm (-march=mipsel)

foo:                                    # @foo
        sync
        addiu   $1, $zero, -4
        and     $1, $4, $1
        andi    $2, $4, 3
        sll     $2, $2, 3
        ori     $3, $zero, 255
        sllv    $3, $3, $2
        nor     $4, $zero, $3
        sllv    $5, $5, $2
$BB0_1:                                 # =>This Inner Loop Header: Depth=1
        ll      $7, 0($1)
        and     $7, $7, $3 # <== WRONG, zero-extending
        and     $5, $5, $3 # <== WRONG, zero-extending 
        slt     $10, $7, $5
        move    $8, $7
        movn    $8, $5, $10
        and     $8, $8, $3
        and     $9, $7, $4
        or      $9, $9, $8
        sc      $9, 0($1)
        beqz    $9, $BB0_1
        nop
        and     $6, $7, $3
        srlv    $6, $6, $2
        sll     $6, $6, 16
        sra     $6, $6, 16
        sync
        jr      $ra
        nop

The problem

The expansion of i8 signed atomic max masks out all bits that are not involved, but fails to sign-extend the values before passing them to slt for a proper signed comparison.

Downstream bug: rust-lang/rust#100650

@llvmbot
Copy link
Member

llvmbot commented Apr 1, 2023

@llvm/issue-subscribers-backend-mips

@yingopq
Copy link
Contributor

yingopq commented Dec 13, 2023

Hi @Amanieu,
What do you think a correct assembly should look like?
$3 has experienced sll, value was (0 | 255) << ((ptr andi 3) << 3). So does this mean expansion?
Thanks.

@Amanieu
Copy link
Contributor Author

Amanieu commented Dec 13, 2023

If you look at the assembly code, it is extracting the byte from the word by masking it (lines marked <== WRONG, zero-extending). The problem is that this is supposed to be a signed atomic max. The values need to be sign-extended before being compared with slt.

@Amanieu
Copy link
Contributor Author

Amanieu commented Dec 13, 2023

The current implementation will happen to work if the byte is the top byte in the word, since no sign-extension is required them. But in other cases the result will be incorrect.

yingopq added a commit to yingopq/llvm-project that referenced this issue Jan 8, 2024
@brad0 brad0 added this to the LLVM 18.0.X Release milestone Jan 21, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Jan 24, 2024
@nikic nikic moved this from Needs Triage to Needs Fix in LLVM Release Status Jan 24, 2024
yingopq added a commit to yingopq/llvm-project that referenced this issue Feb 21, 2024
yingopq added a commit to yingopq/llvm-project that referenced this issue Mar 4, 2024
brad0 pushed a commit that referenced this issue Mar 8, 2024
@github-project-automation github-project-automation bot moved this from Needs Fix to Done in LLVM Release Status Mar 8, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Mar 8, 2024
…llvm#77072)

Add sign extension "SEB/SEH" before compare.

Fix llvm#61881

(cherry picked from commit 755b439)
yingopq added a commit to yingopq/llvm-project that referenced this issue Mar 20, 2024
yingopq added a commit to yingopq/llvm-project that referenced this issue Mar 20, 2024
brad0 pushed a commit that referenced this issue Mar 24, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Mar 24, 2024
…llvm#77072)

Add sign extension "SEB/SEH" before compare.

Fix llvm#61881

(cherry picked from commit 755b439)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Mar 24, 2024
tstellar pushed a commit to llvmbot/llvm-project that referenced this issue Mar 27, 2024
…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 issue Mar 27, 2024
jdmitrovic-syrmia added a commit to jdmitrovic-syrmia/llvm-project that referenced this issue Apr 18, 2024
In order for the following `SLT` instruction to work properly,
we need to sign-extend appropriate subwords.

In addition, subwords must remain in the same position from
before sign-extension.

Resolves llvm#61881. Also, downstream bugs rust-lang/rust#100650 and
rust-lang/rust#123772 are fixed.
jdmitrovic-syrmia added a commit to jdmitrovic-syrmia/llvm-project that referenced this issue Apr 19, 2024
In order for the following `SLT` instruction to work properly,
we need to sign-extend appropriate subwords.

In addition, subwords must remain in the same position from
before sign-extension.

Resolves llvm#61881. Also, downstream bugs rust-lang/rust#100650 and
rust-lang/rust#123772 are fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment