[PATCH] D113783: [InstCombine] Fold (A^B)|~A-->~(A&B)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2676
   // A | (~A ^ B) -> A | ~B
+  // ~A | (A ^ B) -> ~(A & B)
   // (A & B) | (A ^ B)
----------------
MehrHeidar wrote:
> spatel wrote:
> > This surprised me, but this seems to work. Note that 'not' is also an 'xor' operation.
> > But canonicalization rules will always put the 'not' operand on the right (operand 1). So the swap above here then guarantees that the 'not' is operand 0.
> Right, that is exactly the reason that I put the code after the `swap` in line `2671`.  If it is not appropriate I can change it.
I think it's ok, but it would be nice to add the explanation to the code comment. So something like this:
// ~A | (A ^ B) -> ~(A & B) -- The swap above should always make Op0 the 'not'.


================
Comment at: llvm/test/Transforms/InstCombine/or-xor.ll:62
 
+; ~X | (X ^ Y) --> ~(X & Y)
+
----------------
MehrHeidar wrote:
> spatel wrote:
> > We need another test where X and Y are commuted.
> > We also need at least 3 tests with extra uses: (1) extra use of the 'not', (2) extra use of the regular 'xor', (3) extra uses of both of those.
> > The transform should be fine as long there is only one use for either of the operands because we are eliminating an instruction in this sequence.
> > When creating more tests, it's good to vary the data types a bit so we have better coverage. For example, have one test use vector types and another use a weird type like 'i65'.
> Thank you; I have added a few more tests based on your suggestion.
Tests look good. Do you have commit access? If so, please pre-commit the tests with the baseline results, so we will show how the code changes in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113783