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

A miscompilation bug in InstructionSimplify.cpp (select + icmp gep inbounds) #48839

Closed
aqjune opened this issue Mar 9, 2021 · 3 comments
Closed
Assignees
Labels
bugzilla Issues migrated from bugzilla

Comments

@aqjune
Copy link
Contributor

aqjune commented Mar 9, 2021

Bugzilla Link 49495
Resolution FIXED
Resolved on Mar 22, 2021 14:35
Version trunk
OS All
CC @LebedevRI,@nikic,@nunoplopes,@rotateright

Extended Description

$ cat a.ll
define i1 @​f(i8* %a, i8* %b) {
  %cond1 = icmp ne i8* %a, %b
  %a2 = getelementptr inbounds i8, i8* %a, i64 -1
  %cond2 = icmp ugt i8* %a2, %b
  %res = select i1 %cond1, i1 %cond2, i1 false
  ret i1 %res
}

$ opt -instsimplify ./a.ll -S -o -
define i1 @​f(i8* %a, i8* %b) {
  %a2 = getelementptr inbounds i8, i8* %a, i64 -1
  %cond2 = icmp ugt i8* %a2, %b
  ret i1 %cond2
}

This is incorrect: if a = b = null, %res before opt is false whereas the output after opt is poison.

https://alive2.llvm.org/ce/z/SDy_PX

The reason is that SimplifyWithOpReplaced calls SimplifyCmpInst which folds (gep inbounds a, -1) >u a to false even if AllowRefinement is false.

A solution that I came up with is to add 'AllowRefinement' field to SimplifyQuery as well and let SimplifyICmpInst() stop this folding if the flag is set, but I found that SimplifyQuery is used in many places other than InstructionSimplify.
Would it be still a reasonable solution though?

@aqjune
Copy link
Contributor Author

aqjune commented Mar 9, 2021

assigned to @nikic

@aqjune
Copy link
Contributor Author

aqjune commented Mar 11, 2021

A suggested fix: https://reviews.llvm.org/D98391

@aqjune
Copy link
Contributor Author

aqjune commented Mar 22, 2021

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

1 participant