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

Jump to bottom

Sink: moves calls that may not return #51188

Closed
nunoplopes opened this issue Sep 14, 2021 · 2 comments
Closed

Sink: moves calls that may not return #51188

nunoplopes opened this issue Sep 14, 2021 · 2 comments
Labels
bugzilla Issues migrated from bugzilla llvm:optimizations miscompilation

Comments

@nunoplopes
Copy link
Member

Bugzilla Link 51846
Version trunk
OS All
CC @fhahn,@MaskRay

Extended Description

Test: Transforms/Sink/call.ll

Transformation only valid if willreturn & nounwind attributes are present.

define i32 @​test_sink_no_stores(i1 %z) {
%0:
%l = call i32 @​f_load_global() nowrite nofree
br i1 %z, label %true, label %false

%false:
ret i32 0

%true:
ret i32 %l
}
=>
define i32 @​test_sink_no_stores(i1 %z) {
%0:
br i1 %z, label %true, label %false

%false:
ret i32 0

%true:
%l = call i32 @​f_load_global() nowrite nofree
ret i32 %l
}
Transformation doesn't verify!
ERROR: Source is more defined than target

Example:
i1 %z = undef

Source:
i32 %l = UB triggered!

SOURCE MEMORY STATE

NON-LOCAL BLOCKS:
Block 0 > size: 0 align: 1 alloc type: 0
Block 1 > size: 0 align: 1

Target:
UB triggered on br
@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
@nikic nikic closed this as completed in e22af03 Apr 7, 2022
@nikic
Copy link
Contributor

nikic commented Apr 28, 2022

I just realized that there is still a remaining failure:


@A = global 4 bytes, align 4
@B = global 4 bytes, align 4

define i32 @test_sink_argmem_multiple(i1 %z) {
%0:
  %l = call i32 @f_load_arg(ptr @A) nowrite argmemonly nofree willreturn
  call void @f_readonly_arg(ptr @A, ptr @B) argmemonly willreturn
  br i1 %z, label %true, label %false

%false:
  ret i32 0

%true:
  ret i32 %l
}
=>
@A = global 4 bytes, align 4
@B = global 4 bytes, align 4

define i32 @test_sink_argmem_multiple(i1 %z) {
%0:
  call void @f_readonly_arg(ptr @A, ptr @B) argmemonly willreturn
  br i1 %z, label %true, label %false

%false:
  ret i32 0

%true:
  %l = call i32 @f_load_arg(ptr @A) nowrite argmemonly nofree willreturn
  ret i32 %l
}
Transformation doesn't verify!
ERROR: Source is more defined than target

Example:
i1 %z = #x1 (1)

Source:
i32 %l = poison
  >> Jump to %true

SOURCE MEMORY STATE
===================
NON-LOCAL BLOCKS:
Block 0 >	size: 0	align: 1	alloc type: 0
Block 1 >	size: 4	align: 4	alloc type: 0
Block 2 >	size: 4	align: 4	alloc type: 0
Block 3 >	size: 4	align: 4	alloc type: 0

Target:
  >> Jump to %true
i32 %l = UB triggered!

However, I don't really understand what alive is trying to tell me here. Why is this a miscompile? Relevant signatures are:

declare i32 @f_load_arg(i32*) nounwind willreturn readonly argmemonly
declare void @f_readonly_arg(i32* readonly, i32*) nounwind willreturn argmemonly

@nunoplopes
Copy link
Member Author

I think that's a bug in Alive2. Or rather a feature that is not implemented. The call to @f_readonly_arg doesn't have the readonly attribute in the argument.

@nunoplopes nunoplopes mentioned this issue Apr 28, 2022
Closed
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
@nikic
[Sink] Don't sink non-willreturn calls (PR51188)
204304c 
Fixes llvm/llvm-project#51188.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla llvm:optimizations miscompilation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nikic @nunoplopes @EugeneZelenko