[PATCH] D86395: [InstCombine] transform pattern "(~A & B) ^ A -> (A | B)" added

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D86395#2274522 <https://reviews.llvm.org/D86395#2274522>, @Jac1494 wrote:

> In D86395#2269436 <https://reviews.llvm.org/D86395#2269436>, @spatel wrote:
>
>> In D86395#2268634 <https://reviews.llvm.org/D86395#2268634>, @Jac1494 wrote:
>>
>>> test53 and test55 has been removed.
>>
>> We are going in circles. I'll give this 1 more try, and if it still doesn't make sense, then maybe another reviewer can better communicate what we expect for the tests:
>>
>> 1. There are 4 commutative patterns variations in the code.
>> 2. Each one of those patterns should have a test (we should have at least 4 tests).
>> 3. Each test corresponds to exactly one of the commutative patterns.
>> 4. Each test should be in canonical form without this patch (the baseline CHECK lines should correspond exactly to the IR as written).
>> 5. The 8 step check I provided earlier may be used to confirm that the tests provide the expected coverage for the code.
>
> @spatel, apologies for the confusion created due to multiple revisions.
>
> Let's consider the initial pattern and associated commutative version of this.
>
>   initial pattern:-
>   (~A & B) ^ A -> (A | B) 
>   
>   And 8 possible commutative version of this.
>   1) (~A & B) ^ A -> (A | B) 
>   2) (~B & A) ^ B -> (A | B)
>   3) (B & ~A) ^ A -> (A | B)  --> identical to 1)
>   4) A ^ (~A & B) -> (A | B)  --> identical to 1)
>   5) A ^ (B & ~A) -> (A | B)  --> identical to 1)
>   6) (A & ~B) ^ B -> (A | B)  --> identical to 2)
>   7) B ^ (~B & A) -> (A | B)  --> identical to 2)
>   8) B ^ (A & ~B) -> (A | B)  --> identical to 2)
>
> As you may notice that all these patterns (IR) are reducing to 2 unique patterns (pattern 1 and pattern 2).
> So pattern 1 and 2 are sufficient for the coverage of all commutative variations of the primary pattern.  
> Does this matches with you understandings ??

No. First, there are not 8 unique patterns. There are 4. For the sake of consistency with the code that you have proposed, you can always think of "A" as the operand with a `not`. Renaming that to "B" or "Foo" or anything else does not change the logic.
Second, I don't think you understand how commutative canonicalization works - you should put a breakpoint in here and step through in a debugger:
https://github.com/llvm/llvm-project/blob/4452cc4086aca1a424b2cd40da9fa120add522e7/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h#L126
Third, you ignored my earlier suggestion to open a review to add/modify the tests alone. Please do that now, so we can reach a conclusion sooner.


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

https://reviews.llvm.org/D86395