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

urem+udiv miscompile with global isel on arm64 #55287

Closed
regehr opened this issue May 5, 2022 · 6 comments
Closed

urem+udiv miscompile with global isel on arm64 #55287

regehr opened this issue May 5, 2022 · 6 comments

Comments

@regehr
Copy link
Contributor

regehr commented May 5, 2022

this is being miscompiled by the arm64 backend with global isel:

define i32 @test1(i32 %0) {
  %2 = urem i32 %0, %0
  %3 = udiv i32 %2, %0
  ret i32 %3
}

we want it to just return 0 (or something else that always produces 0) but we get:

regehr@john-home:~$ llc foo.ll -o - -global-isel --march=arm64
	.text
	.file	"foo.ll"
	.globl	test1                           // -- Begin function test1
	.p2align	2
	.type	test1,@function
test1:                                  // @test1
	.cfi_startproc
// %bb.0:
	udiv	w0, w0, w0
	ret
.Lfunc_end0:
	.size	test1, .Lfunc_end0-test1
	.cfi_endproc
                                        // -- End function
	.section	".note.GNU-stack","",@progbits
regehr@john-home:~$ 

cc @ornata @nunoplopes @ryan-berger @nbushehri @zhengyang92 @aqjune @Hatsunespica

@llvmbot
Copy link
Member

llvmbot commented May 5, 2022

@llvm/issue-subscribers-backend-aarch64

@regehr
Copy link
Contributor Author

regehr commented May 5, 2022

we're also seeing versions of this bug with signed operations

regehr@john-home:~$ cat foo.ll
define i8 @test_sdiv8(i8 %0, i8 %1) #0 {
  %3 = srem i8 %1, %1
  %4 = sdiv i8 %3, %1
  ret i8 %4
}
regehr@john-home:~$ llc foo.ll -o - -global-isel --march=arm64
	.text
	.file	"foo.ll"
	.globl	test_sdiv8                      // -- Begin function test_sdiv8
	.p2align	2
	.type	test_sdiv8,@function
test_sdiv8:                             // @test_sdiv8
	.cfi_startproc
// %bb.0:
	sxtb	w8, w1
	sdiv	w0, w8, w8
	ret
.Lfunc_end0:
	.size	test_sdiv8, .Lfunc_end0-test_sdiv8
	.cfi_endproc
                                        // -- End function
	.section	".note.GNU-stack","",@progbits
regehr@john-home:~$ 

@nicolas17
Copy link
Contributor

Testing the original code in compiler explorer:

define i32 @test1(i32 %0) {
  %2 = urem i32 %0, %0
  %3 = udiv i32 %2, %0
  ret i32 %3
}

LLVM 11 outputs:

        udiv    w8, w0, w0
        mul     w8, w8, w0
        subs    w8, w0, w8
        udiv    w0, w8, w0
        ret

LLVM 12 outputs:

        udiv    w8, w0, w0
        msub    w8, w8, w0, w0
        udiv    w0, w8, w0
        ret

I think both of these are correct and always result in 0.

LLVM 13 outputs:

        udiv    w0, w0, w0
        ret

So, it's bisect time...

@nicolas17
Copy link
Contributor

Bisect result:

In 4c6ab48, instead of emitting the udiv+msub+udiv posted above, it started failing altogether with LLVM ERROR: unable to legalize instruction: %2:_(s32), %1:_ = G_UDIVREM %0:_, %0:_ (in function: test1).

In ac1647c (3986 commits later), it stopped erroring out in the legalizer and started emitting udiv w0, w0, w0.

@ornata
Copy link

ornata commented May 11, 2022

cc @TNorthover

@aemerson aemerson self-assigned this Jul 25, 2022
aemerson added a commit that referenced this issue Jul 25, 2022
…r equality

of the first operands of each.

Fixes issue #55287

Differential Revision: https://reviews.llvm.org/D130525
@aemerson
Copy link
Contributor

https://reviews.llvm.org/D130525 and committed in 5ae0472

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants