[PATCH] D151694: [InstCombine] Add test case for (icmp eq A, -1) & (icmp eq B, -1) --> (icmp eq (A&B), -1); NFC

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D151694#4391257 <https://reviews.llvm.org/D151694#4391257>, @xgupta wrote:

> In D151694#4389823 <https://reviews.llvm.org/D151694#4389823>, @goldstein.w.n wrote:
>
>> Please regenerate the tests here + in the implementation (I think you forgot to when you switched the parent/child relationship).
>
> Yes, I am not sure about how parent/child revisions work.

No problem. It really is there to represent multiple dependent commits.
So what we want to get submitted to the project is Commit A and Commit B.

Commit A: Tests
Commit B: Implementation

Commit B depends on commit A in the sense that it doesn't apply to head, it has diffs based on commit A.
parent/child is there to represent that relationship.

> I will update the patch soon, seems the test cases are not correct for this patch, even without change there is icmp fold happening.
>
> Without change -
>
>   define i1 @icmp_eq_m1_and_eq_m1(i8 %x, i8 %y) {
>   ; CHECK-LABEL: @icmp_eq_m1_and_eq_m1(
>   ; CHECK-NEXT:    [[TMP1:%.*]] = and i8 [[X:%.*]], [[Y:%.*]]
>   ; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[TMP1]], -1
>   ; CHECK-NEXT:    ret i1 [[R]]
>   ;
>     %rx = icmp eq i8 %x, -1
>     %ry = icmp eq i8 %y, -1
>     %r = and i1 %rx, %ry
>     ret i1 %r
>   }  

It seems that only the vector test '@allones' still changes as a result of this patch. This means that
the code is implemented somewhere else but it is not able to handle all types (this is probably
what nikic's comment was about: https://alive2.llvm.org/ce/z/CN7Cgd).
The code currently exists in: `foldLogOpOfMaskedICmps` (specifically the `if (Mask & BMask_AllOnes) {` and `if (Mask & AMask_AllOnes) {` cases).

I'm not sure why it doesn't work for the '@allones' version, it seems get about half the combines, but it fails early.

Can you test your patch on the following codes:

  define <2 x i1> @icmp_eq_m1_and_eq_m1(<2 x i8> %x, <2 x i8> %y) {
    %rx = icmp eq <2 x i8> %x, <i8 -1, i8 undef>
    %ry = icmp eq <2 x i8> %y, <i8 -1, i8 undef>
    %r = and <2 x i1> %rx, %ry
    ret <2 x i1> %r
  }

?

That should work with your patch and not without it. If so update your tests with that pattern (partially undef vecs).
Please add tests, however, for partial undef vecs where the undef elements don't match: https://alive2.llvm.org/ce/z/P83Svp

Then please update the todo in nikics code to say "remove this *and below*...."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151694