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.

Sign up for GitHub

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

[SDAG] Miscompile of logical or of comparisons of loads with !range metadata #64589

Closed
nikic opened this issue Aug 10, 2023 · 6 comments · Fixed by llvm/llvm-project-release-prs#580
Closed

[SDAG] Miscompile of logical or of comparisons of loads with !range metadata #64589

nikic opened this issue Aug 10, 2023 · 6 comments · Fixed by llvm/llvm-project-release-prs#580
Assignees
@nikic
Labels
llvm:codegen miscompilation release:backport release:merged
Milestone
LLVM 17.0.X Release

Comments

@nikic
Copy link
Contributor

nikic commented Aug 10, 2023 •
edited by VoltrexKeyva
Loading 

define i8 @test(ptr %p) {
  %v1 = load i8, ptr %p, align 4, !range !0, !noundef !{}
  %cmp1 = icmp eq i8 %v1, 0
  %p2 = getelementptr inbounds i8, ptr %p, i64 1
  %v2 = load i8, ptr %p2, align 1, !range !0
  %cmp2 = icmp eq i8 %v2, 0
  %or = select i1 %cmp1, i1 %cmp2, i1 false
  %res = select i1 %or, i8 0, i8 2
  ret i8 %res
}

!0 = !{i8 0, i8 2}

Lowers to:

	movzbl	(%rdi), %eax
	orb	1(%rdi), %al
	addb	%al, %al
	retq

This is incorrect because the %v2 only has !range, but not !noundef. As such, the load may return a poison value.

I believe nominally the folds at https://github.com/llvm/llvm-project/blob/68744ffbdd7daac41da274eef9ac0d191e11c16d/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L11252 are at fault here: These select to and/or folds are well-known to be unsound in the presence of poison values.

However, removing these folds would be quite involved (as we have seen on the IR side), so the immediate fix here is probably to not transfer !range metadata to SDAG if it does not have !noundef, which sidesteps the issue.
@nikic nikic added this to the LLVM 17.0.X Release milestone Aug 10, 2023
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Aug 10, 2023
@nikic nikic moved this from Needs Triage to Needs Fix in LLVM Release Status Aug 10, 2023
@nikic nikic mentioned this issue Aug 10, 2023
Closed
@nikic nikic self-assigned this Aug 10, 2023
@nikic
Copy link
Contributor Author

nikic commented Aug 11, 2023

Candidate patch: https://reviews.llvm.org/D157685

@nikic nikic closed this as completed in 9deee6b Aug 14, 2023
@nikic nikic reopened this Aug 14, 2023
@nikic
Copy link
Contributor Author

nikic commented Aug 14, 2023

/cherry-pick 9deee6b

@llvmbot
Copy link

llvmbot commented Aug 14, 2023

Failed to cherry-pick: 9deee6b

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

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>

@nikic
Copy link
Contributor Author

nikic commented Aug 14, 2023

/cherry-pick 59d558a 9deee6b

@llvmbot
Copy link

llvmbot commented Aug 14, 2023

/branch llvm/llvm-project-release-prs/issue64589

llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 14, 2023
@nikic @llvmbot
[SDAG] Don't transfer !range metadata without !noundef to SDAG (PR64589)
3600abf 
D141386 changed the semantics of !range metadata to return poison
on violation. If !range is combined with !noundef, violation is
immediate UB instead, matching the old semantics.

In theory, these IR semantics should also carry over into SDAG.
In practice, DAGCombine has at least one key transform that is
invalid in the presence of poison, namely the conversion of logical
and/or to bitwise and/or (https://github.com/llvm/llvm-project/blob/c7b537bf0923df05254f9fa4722b298eb8f4790d/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L11252).
Ideally, we would fix this transform, but this will require
substantial work to avoid codegen regressions.

In the meantime, avoid transferring !range metadata without
!noundef, effectively restoring the old !range metadata semantics
on the SDAG layer.

Fixes llvm/llvm-project#64589.

Differential Revision: https://reviews.llvm.org/D157685

(cherry picked from commit 9deee6b)
@llvmbot llvmbot mentioned this issue Aug 14, 2023
Merged
@llvmbot
Copy link

llvmbot commented Aug 14, 2023

/pull-request llvm/llvm-project-release-prs#580

@tru tru moved this from Needs Fix to Needs Review in LLVM Release Status Aug 14, 2023
@nikic nikic moved this from Needs Review to Needs Merge in LLVM Release Status Aug 14, 2023
tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 15, 2023
@nikic @tru
[SDAG] Don't transfer !range metadata without !noundef to SDAG (PR64589)
f124907 
D141386 changed the semantics of !range metadata to return poison
on violation. If !range is combined with !noundef, violation is
immediate UB instead, matching the old semantics.

In theory, these IR semantics should also carry over into SDAG.
In practice, DAGCombine has at least one key transform that is
invalid in the presence of poison, namely the conversion of logical
and/or to bitwise and/or (https://github.com/llvm/llvm-project/blob/c7b537bf0923df05254f9fa4722b298eb8f4790d/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L11252).
Ideally, we would fix this transform, but this will require
substantial work to avoid codegen regressions.

In the meantime, avoid transferring !range metadata without
!noundef, effectively restoring the old !range metadata semantics
on the SDAG layer.

Fixes llvm/llvm-project#64589.

Differential Revision: https://reviews.llvm.org/D157685

(cherry picked from commit 9deee6b)
@tru tru moved this from Needs Merge to Done in LLVM Release Status Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nikic nikic

Labels
llvm:codegen miscompilation release:backport release:merged
Projects
LLVM Release Status
Status: Done
Milestone
LLVM 17.0.X Release
Development

Successfully merging a pull request may close this issue.

PR for llvm/llvm-project#64589 llvm/llvm-project-release-prs
3 participants
@nikic @EugeneZelenko @llvmbot