[PATCH] D118094: [ValueTracking] Checking haveNoCommonBitsSet for (x & y) and ~(x | y)

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:290
+    Value *A, *B;
+    if (match(LHS, m_c_And(m_Value(A), m_Value(B))) &&
+        match(RHS, m_Not(m_c_Or(m_Specific(A), m_Specific(B)))))
----------------
spatel wrote:
> If I'm seeing it correctly, these lines could use "m_And" rather than "m_c_And"?
Oh yes. Great Catcha.


================
Comment at: llvm/test/Transforms/InstCombine/pr53357.ll:19-20
 
 ; (x & y) + ~(x | y)
 define i32 @src_thwart(i32 %0, i32 %1) {
 ; CHECK-LABEL: @src_thwart(
----------------
spatel wrote:
> This test does not add any value to the previous test. Let's try to simplify things.
> 
> IIUC, there are exactly 4 commutative patterns that we want to match:
>   (x & y) + ~(x | y)
>   (x & y) + ~(y | x) 
>   ~(x | y) + (x & y) 
>   ~(y | x) + (x & y)
> 
> (We can swap the names of x and y, and it doesn't change any logic in the matching code.)
> 
> 1. Create tests that correspond to those patterns (including using the same value names as in the test comments):
> 
> ```
> ; (x & y) + ~(x | y)
> define i32 @no_common_bits_and_nor(i32 %x, i32 %y) {
>   %and = and i32 %x, %y
>   %or = or i32 %x, %y
>   %nor = xor i32 %or, -1
>   %r = add i32 %and, %nor
>   ret i32 %r
> }
> ```
> 
> 2. You can vary the types while checking the commutes, so some of the tests can use vectors (with or without an undef in the 'not' constant).
> 
> 3. I'm not sure what the tests with extra 'not' instructions are showing. As we can see in the baseline CHECK lines, those are transformed before we reach the transform in this patch. If we want to show that this patch can or can't handle an unreduced logic pattern, then you have to add extra uses of the intermediate values to prevent the other transforms from altering the instructions in the tests.
> This test does not add any value to the previous test. Let's try to simplify things.

Do you mean the *_thwart tests is meaningless? Or the current test doesn't add value with the pre-committed test?

 > Create tests that correspond to those patterns (including using the same value names as in the test comments):

I think we have done this in the patch. Do you feel not good about the name? (Use no_common_bits_and_nor instead of src)

> You can vary the types while checking the commutes, so some of the tests can use vectors (with or without an undef in the 'not' constant).

Yeah, we made this too. Do you say it would be better if every test has a vector version (and corresponding undef version)?

> I'm not sure what the tests with extra 'not' instructions are showing. As we can see in the baseline CHECK lines, those are transformed before we reach the transform in this patch.

Do you talk about the case:  `(x & y) + (~x & ~y)`? I added this from the discussion in: https://github.com/llvm/llvm-project/issues/53357. It said the form could be converted correctly after we finish the patch. Do you think the test is redundant?


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

https://reviews.llvm.org/D118094