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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
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(
----------------
ChuanqiXu wrote:
> spatel wrote:
> > ChuanqiXu wrote:
> > > 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?
> > I think this particular test is redundant, but we do need some "thwart" tests too. It's possible that all of the interesting patterns are covered by the tests right now, but it is difficult for me to see it. We do not need to repeat everything with vectors or vector+undef. We can reduce to the essential 4 commuted patterns and vary the types within those. That would be the most efficient set (4) of tests.
> > 
> > It's not necessary to include the (x & y) + (~x & ~y) patterns in this patch. We will reduce those to the minimum code independently using DeMorgan transforms. 
> > 
> > If we want to show a limitation of this patch (when the intermediate 'not' values have extra uses), then it is fine to include more tests, but they will not be changed with this patch unless I did not understand correctly.
> Got it. Thanks. The last question is what's the reason that you think the *_thwart tests redundant. After comparing with existing example, I found a possible reason might be that I thwarted more value. Should I thwart %x only instead of thwarting both %x and %y?
> Or what's the particular reason?
The baseline CHECK lines show that the pattern is identical with or without the extra instructions on this pair of tests ("src" and "src_thwart").

In fact, none of the thwart variations makes any difference on these examples. Sorry that I didn't notice this sooner, but we can not test this pattern within instcombine:
  ~(x | y) + (x & y)

That's because:
https://github.com/llvm/llvm-project/blob/d224be3b999afb7c4daa9c0ca807dea8123a7593/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h#L127

InstCombine will always commute the 'not' operation to be operand 1 (right-hand-side) because the other operation is an "and". You can see this in "src5".

ValueTracking may be called from other passes, so we probably want to keep the code as-is, but you can not test the variation where 'not' is operand 0 from within instcombine (it's fine to include a test to make sure that doesn't change in the future).


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

https://reviews.llvm.org/D118094