The following instcombine rewrite seems incorrect (test/Transforms/InstCombine/canonicalize-constant-low-bit-mask-and-icmp-sgt-to-icmp-sgt.ll): define i1 @cv0_GOOD(i8 %y) { %0: %x = call i8 @gen8() %tmp0 = lshr i8 255, %y %tmp1 = and i8 %tmp0, %x %ret = icmp sgt i8 %x, %tmp1 ret i1 %ret } => define i1 @cv0_GOOD(i8 %y) { %0: %x = call i8 @gen8() %tmp0 = lshr i8 255, %y %1 = icmp sgt i8 %x, %tmp0 ret i1 %1 } Transformation doesn't verify! ERROR: Value mismatch Example: i8 %y = #x00 (0) Source: i8 %x = #x7f (127, -129) i8 %tmp0 = #xff (255, -1) i8 %tmp1 = #x7f (127, -129) i1 %ret = #x0 (0) Target: i8 %x = #x7f (127, -129) i8 %tmp0 = #xff (255, -1) i1 %1 = #x1 (1) Source value: #x0 (0) Target value: #x1 (1)
Nice! Thank you for reporting! Really happy that the tool, as it is being expanded, uncovers yet more and more horrors of all this pattern-matching! And same for llvm/test/Transforms/InstCombine/canonicalize-constant-low-bit-mask-and-icmp-sle-to-icmp-sle.ll So basically all the folds in that group when the comparison predicate is signed.
For reference, the commit that introduced this specific test change was: https://reviews.llvm.org/rL337105 We missed a precondition. Not sure if it's worth using value tracking (isKnownNonZero) to preserve the cases that could still be reduced: https://rise4fun.com/Alive/5gkT Pre: C1 != 0 %tmp0 = lshr i8 255, C1 %tmp1 = and i8 %tmp0, %x %ret = icmp sgt i8 %x, %tmp1 => %tmp0 = lshr i8 255, C1 %ret = icmp sgt i8 %x, %tmp0
(In reply to Sanjay Patel from comment #2) > For reference, the commit that introduced this specific test change was: > https://reviews.llvm.org/rL337105 Yep. > We missed a precondition. Not so much missed. I did check the original fold with constants, and then somehow managed to be completely unbothered to check the fold with the `lshr`, and didn't think of it even. > Not sure if it's worth using value tracking > (isKnownNonZero) to preserve the cases that could still be reduced: Could be done as a new feature. > https://rise4fun.com/Alive/5gkT > > Pre: C1 != 0 > %tmp0 = lshr i8 255, C1 > %tmp1 = and i8 %tmp0, %x > %ret = icmp sgt i8 %x, %tmp1 > => > %tmp0 = lshr i8 255, C1 > %ret = icmp sgt i8 %x, %tmp0
Checked other folds there, with other edge-cases (0, -1), everything else seems fine now.. Thanks for the bugreport (: