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

[Mehrnoosh Heidarpour via Phabricator via llvm-commits]

MehrHeidar marked 4 inline comments as done.
MehrHeidar 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)
----------------
spatel wrote:
> 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'.
Thank you; I have added your suggested comment.


================
Comment at: llvm/test/Transforms/InstCombine/or-xor.ll:62
 
+; ~X | (X ^ Y) --> ~(X & Y)
+
----------------
spatel wrote:
> 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.
No, I do not have commit access; But I prepared another patch to pre-commit the tests with baseline results here: https://reviews.llvm.org/D113846

If it's possible, Can you please commit it for me?
Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113783