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

[PowerPC] Incorrect and/anyext interchange in custom combine #68783

Closed
nikic opened this issue Oct 11, 2023 · 6 comments · Fixed by #68784 or llvm/llvm-project-release-prs#731
Closed

[PowerPC] Incorrect and/anyext interchange in custom combine #68783

nikic opened this issue Oct 11, 2023 · 6 comments · Fixed by #68784 or llvm/llvm-project-release-prs#731
Assignees
@nikic
Labels
backend:PowerPC miscompilation release:backport release:merged
Milestone
LLVM 17.0.X Release

Comments

@nikic
Copy link
Contributor

nikic commented Oct 11, 2023 

; RUN: llc -mtriple=powerpc64le-unknown-unknown -ppc-asm-full-reg-names < %s
define void @test(i32 %x, ptr %p) {
  %lshr = lshr i32 %x, 1
  %zext = zext i32 %lshr to i48
  %and = and i48 %zext, 255
  store i48 %and, ptr %p
  ret void
}

Results in:

	rlwinm r3, r3, 31, 24, 31
	stw r3, 0(r4)
	blr

Note the missing store of the upper 16 bits.

This is caused by an incorrect custom DAGCombine added in b0e249d.
@nikic nikic added this to the LLVM 17.0.X Release milestone Oct 11, 2023
@nikic nikic self-assigned this Oct 11, 2023
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Oct 11, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 11, 2023

@llvm/issue-subscribers-backend-powerpc

Author: Nikita Popov (nikic)

```llvm ; RUN: llc -mtriple=powerpc64le-unknown-unknown -ppc-asm-full-reg-names < %s define void @test(i32 %x, ptr %p) { %lshr = lshr i32 %x, 1 %zext = zext i32 %lshr to i48 %and = and i48 %zext, 255 store i48 %and, ptr %p ret void } ``` Results in: ``` rlwinm r3, r3, 31, 24, 31 stw r3, 0(r4) blr ``` Note the missing store of the upper 16 bits.

This is caused by an incorrect custom DAGCombine added in b0e249d.

@nikic nikic mentioned this issue Oct 11, 2023
Closed
nikic added a commit that referenced this issue Oct 11, 2023
@nikic
[PowerPC] Add test for #68783 (NFC)
0ead1fa
nikic added a commit to nikic/llvm-project that referenced this issue Oct 11, 2023
@nikic
[PowerPC] Use zext instead of anyext in custom and combine
c3b6edd 
This custom combine currently converts an `and(anyext(x),c)`
into `anyext(and(x,c))`. This is not correct, because the original
expression guaranteed that the high bits are zero, while the new
one sets them to undef.

Emit `zext(and(x,c))` instead.

Fixes llvm#68783.
@nikic nikic mentioned this issue Oct 11, 2023
Merged
@tru tru moved this from Needs Triage to Needs Review in LLVM Release Status Oct 11, 2023
@tru tru moved this from Needs Review to Needs Pull Request in LLVM Release Status Oct 12, 2023
@nikic nikic reopened this Oct 12, 2023
@nikic
Copy link
Contributor Author

nikic commented Oct 12, 2023

/cherry-pick 0ead1fa 127ed9a

@llvmbot
Copy link
Member

llvmbot commented Oct 12, 2023

/branch llvm/llvm-project-release-prs/issue68783

llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Oct 12, 2023
@nikic @llvmbot
[PowerPC] Use zext instead of anyext in custom and combine (#68784)
ca676a5 
This custom combine currently converts `and(anyext(x),c)` into
`anyext(and(x,c))`. This is not correct, because the original expression
guaranteed that the high bits are zero, while the new one sets them to
undef.

Emit `zext(and(x,c))` instead.

Fixes llvm/llvm-project#68783.

(cherry picked from commit 127ed9ae266ead58aa525f74f4c86841f6674793)
@llvmbot llvmbot mentioned this issue Oct 12, 2023
Closed
@llvmbot
Copy link
Member

llvmbot commented Oct 12, 2023

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

nikic added a commit to nikic/llvm-project that referenced this issue Oct 13, 2023
@nikic
[PowerPC] Add test for llvm#68783 (NFC)
f06e645 
(cherry picked from commit 0ead1fa)
nikic added a commit to nikic/llvm-project that referenced this issue Oct 13, 2023
@nikic
[PowerPC] Use zext instead of anyext in custom and combine (llvm#68784)
6ad1f9b 
This custom combine currently converts `and(anyext(x),c)` into
`anyext(and(x,c))`. This is not correct, because the original expression
guaranteed that the high bits are zero, while the new one sets them to
undef.

Emit `zext(and(x,c))` instead.

Fixes llvm#68783.

(cherry picked from commit 127ed9a)
@nikic
Copy link
Contributor Author

nikic commented Oct 13, 2023

/branch nikic/llvm-project/powerpc-backport

@llvmbot llvmbot mentioned this issue Oct 13, 2023
Merged
@llvmbot
Copy link
Member

llvmbot commented Oct 13, 2023

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

@owenca owenca closed this as completed Oct 15, 2023
@nikic nikic reopened this Oct 16, 2023
@tru tru moved this from Needs Pull Request to Needs Review in LLVM Release Status Oct 16, 2023
tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Oct 17, 2023
@nikic @tru
[PowerPC] Use zext instead of anyext in custom and combine (#68784)
491a91e 
This custom combine currently converts `and(anyext(x),c)` into
`anyext(and(x,c))`. This is not correct, because the original expression
guaranteed that the high bits are zero, while the new one sets them to
undef.

Emit `zext(and(x,c))` instead.

Fixes llvm/llvm-project#68783.

(cherry picked from commit 127ed9ae266ead58aa525f74f4c86841f6674793)
tru pushed a commit that referenced this issue Oct 17, 2023
@nikic @tru
[PowerPC] Add test for #68783 (NFC)
8ce6b65 
(cherry picked from commit 0ead1fa)
@tru tru moved this from Needs Review to Done in LLVM Release Status Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nikic nikic

Labels
backend:PowerPC miscompilation release:backport release:merged
Projects
LLVM Release Status
Status: Done
Milestone
LLVM 17.0.X Release
3 participants
@nikic @EugeneZelenko @llvmbot