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

likely wrong code bug from arm64 backend with global isel enabled #55003

Closed
regehr opened this issue Apr 20, 2022 · 13 comments
Closed

likely wrong code bug from arm64 backend with global isel enabled #55003

regehr opened this issue Apr 20, 2022 · 13 comments

Comments

@regehr
Copy link
Contributor

regehr commented Apr 20, 2022

here's a function that's part of the LLVM unit test suite:

; ModuleID = 'file-020267.bc'
source_filename = "/home/regehr/llvm-project/llvm/test/Transforms/InstCombine/fsh.ll"

target triple = "arm64-apple-macosx12.0.0"

; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
declare i32 @llvm.fshl.i32(i32, i32, i32) #0

define i32 @fshl_undef_shift_amount(i32 %x, i32 %y) {
  %r = call i32 @llvm.fshl.i32(i32 %x, i32 %y, i32 undef)
  ret i32 %r
}

attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }

here's the output we get from LLVM 14 and also top of tree:

Johns-MacBook-Pro:~ regehr$ llc foo.ll -global-isel -march=arm64 -o -
	.section	__TEXT,__text,regular,pure_instructions
	.build_version macos, 12, 0
	.globl	_fshl_undef_shift_amount        ; -- Begin function fshl_undef_shift_amount
	.p2align	2
_fshl_undef_shift_amount:               ; @fshl_undef_shift_amount
	.cfi_startproc
; %bb.0:
	mov	w8, #32
	udiv	w9, w8, w8
	msub	w9, w9, w8, w8
	sub	w8, w8, w9
	lsl	w9, w0, w9
	lsr	w8, w1, w8
	orr	w0, w9, w8
	ret
	.cfi_endproc
                                        ; -- End function
.subsections_via_symbols
Johns-MacBook-Pro:~ regehr$ 

this function, called with arguments (0, 4) returns 4, which is not one of the permitted outputs given how undef works. unless we're missing something, permissible outputs are 0, 1, 2.

here's the driver I'm using:

#include <stdio.h>

unsigned fshl_undef_shift_amount(unsigned, unsigned);

int main(void) {
  unsigned z = fshl_undef_shift_amount(0, 4);
  printf("%u\n", z);
  return 0;
}

and the output:

Johns-MacBook-Pro:~ regehr$ clang foo.s foo.c 
Johns-MacBook-Pro:~ regehr$ ./a.out 
4
Johns-MacBook-Pro:~ regehr$ 

this was found using a highly preliminary version of alive2 that does translation validation for the arm64 backend.
cc @nunoplopes @nbushehri @ryan-berger

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 20, 2022

@llvm/issue-subscribers-backend-arm

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 20, 2022

@llvm/issue-subscribers-backend-aarch64

1 similar comment
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 20, 2022

@llvm/issue-subscribers-backend-aarch64

@nunoplopes
Copy link
Member

@EugeneZelenko concurrency clash, sorry :)

@EugeneZelenko
Copy link
Contributor

@nunoplopes: It's fine :-)

@fhahn
Copy link
Contributor

fhahn commented Apr 20, 2022

cc @aemerson @ornata

@davemgreen
Copy link
Collaborator

davemgreen commented Apr 21, 2022

this was found using a highly preliminary version of alive2 that does translation validation for the arm64 backend.

Sounds very interesting. Do you have a link or a paper yet?

@regehr
Copy link
Contributor Author

regehr commented Apr 21, 2022

Sounds very interesting. Do you have a link or a paper yet?

nothing yet, still very much a work in progress. we hope to give a talk at the next LLVM Dev meeting and of course would be happy to discuss in person there or elsewhere

@nicolas17
Copy link
Contributor

fwiw 7bddf00 generates the same asm so this is not a regression.

@dzhidzhoev
Copy link
Member

Seems to be fixed by https://reviews.llvm.org/D125041

@tstellar tstellar added this to the LLVM 15.0.0 Release milestone Aug 1, 2022
@tstellar
Copy link
Collaborator

tstellar commented Aug 8, 2022

/cherry-pick 41fef10

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 8, 2022

Failed to cherry-pick: 41fef10

https://github.com/llvm/llvm-project/actions/runs/2821599508

Please manually backport the fix and push it to your github fork. Once this is done, please add a comment like this:

/branch <user>/<repo>/<branch>

@efriedma-quic
Copy link
Collaborator

The patch landed months ago; it's already in llvm 15.

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

No branches or pull requests

10 participants