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

foldSelectICmpAndAnd should check whether the shift amount is less than bitwidth #47012

Closed
aqjune opened this issue Sep 28, 2020 · 2 comments
Closed
Labels

Comments

@aqjune
Copy link
Contributor

aqjune commented Sep 28, 2020

Bugzilla Link 47668
Version trunk
OS MacOS X
CC @LebedevRI,@RKSimon,@nunoplopes,@rotateright

Extended Description

File: test/Transforms/InstCombine/select-of-bittest.ll

define i32 @​f_var2(i32 %arg, i32 %arg1) {
%0:
  %tmp = and i32 %arg, 1
  %tmp2 = icmp eq i32 %tmp, 0
  %tmp3 = lshr i32 %arg, %arg1
  %tmp4 = and i32 %tmp3, 1
  %tmp5 = select i1 %tmp2, i32 %tmp4, i32 1
  ret i32 %tmp5
}
=>
define i32 @​f_var2(i32 %arg, i32 %arg1) {
%0:
  %1 = shl i32 1, %arg1
  %2 = or i32 %1, 1
  %3 = and i32 %2, %arg
  %4 = icmp ne i32 %3, 0
  %tmp5 = zext i1 %4 to i32
  ret i32 %tmp5
}
Transformation doesn't verify!
ERROR: Target is more poisonous than source

Example:
i32 %arg = #x00000001 (1)
i32 %arg1 = poison

Source:
i32 %tmp = #x00000001 (1)
i1 %tmp2 = #x0 (0)
i32 %tmp3 = poison
i32 %tmp4 = poison
i32 %tmp5 = #x00000001 (1)

Target:
i32 %1 = poison
i32 %2 = poison
i32 %3 = poison
i1 %4 = poison
i32 %tmp5 = poison
Source value: #x00000001 (1)
Target value: poison

Relevant revision: https://reviews.llvm.org/D45108
The revision includes Alive proof, but its precondition wasn't encoded.
I'll make a patch for this.

@aqjune
Copy link
Contributor Author

aqjune commented Sep 28, 2020

Made a patch here: https://reviews.llvm.org/D88432

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
rotateright added a commit that referenced this issue Jul 1, 2022
This transform is responsible for a long-standing miscompile
as discussed in issue #47012 (was bugzilla #47668).

There was a proposal to correct it in D88432, but that was
abandoned and there hasn't been any recent activity to fix
it AFAICT.

The original patch D45108 started with a constant-shift-only
restriction and only expanded during review, so I don't think
there's much risk of perf regression on the motivating code.
@rotateright
Copy link
Contributor

Restricted the fold with a constant value check:
9c8a39c

I'm not sure how we would safely implement the fold with a variable shift amount, so I put a TODO comment there in case someone wants to revisit it.

I think that fixes all tests that were miscompiles. If I got that wrong, please re-open.

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

No branches or pull requests

3 participants