[PATCH] D132989: [InstSimplify] Odd - X ==/!= X -> false/true

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D132989#3766063 <https://reviews.llvm.org/D132989#3766063>, @liaolucy wrote:

>> I think we can generalize the transform at least in one direction - the constant does not have to be 1:
>> https://alive2.llvm.org/ce/z/bPYNJT
>> (not sure if there's some other relationship for non-equality predicates)
>
> I am trying to understand the example， get odd constant value from more instructions(>1), I don't know how to implement it, any suggestions?

The proof just shows that the transform is valid for any value with low-bit cleared.

If the value is a constant, then it is easy to detect. If the value is not constant, then we have to try more expensive (in compile-time) analysis - see ValueTracking's computeKnownBits() for example.

If we do not have a real-world example that shows the more expensive transform is needed, then we should not add it. In other words, the patch is mostly fine as implemented. But I do not understand why we have no-wrap requirements. The transform is correct without those:
https://alive2.llvm.org/ce/z/h4ZFVD



================
Comment at: llvm/test/Transforms/InstCombine/icmp-sub.ll:570
+;
+entry:
+  %sub = sub i32 1, %x
----------------
No need to include "entry" label in a minimum test like this.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-sub.ll:572
+  %sub = sub i32 1, %x
+  %cmp = icmp eq i32 %sub, %x
+  ret i1 %cmp
----------------
What happens if %sub and %x are swapped in this test?


================
Comment at: llvm/test/Transforms/InstCombine/icmp-sub.ll:576
+
+define i1 @sub_nsw_odd(i8 %x, i8 %c) {
+; CHECK-LABEL: @sub_nsw_odd(
----------------
Change this to a vector type to confirm that the transform works in that case too. We should have one test where the vector constant includes a "poison" element too.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-sub.ll:587
+
+define i1 @sub_nuw_odd(i8 %x, i8 %c) {
+; CHECK-LABEL: @sub_nuw_odd(
----------------
We need at least one negative test with an even constant. That shows that the transform is not happening when it should not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132989/new/

https://reviews.llvm.org/D132989