[PATCH] D16978: [InstCombine] Try harder to simplify ~(X & Y) -> ~X | ~Y and ~(X | Y) -> ~X & ~Y when X and Y have more than one uses.

[Chad Rosier via llvm-commits]

mcrosier added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1344
@@ +1343,3 @@
+  // the ~V.
+  if (!isa<CmpInst>(V) && !isa<BinaryOperator>(V))
+    return false;
----------------
Perhaps you could move the isa<BinaryOperator> check into a separate if statement.  IMO, it doesn't go with the comment above.

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1349
@@ +1348,3 @@
+  // - Constant) - A` if we are willing to invert some of the uses.
+  if (BinaryOperator *BO = dyn_cast<BinaryOperator>(V))
+    if (BO->getOpcode() == Instruction::Add ||
----------------
This can be a cast<> rather than a dyn_cast<> because you've already checked we have a BO.

Alternatively, you could do the following to address the previous comment:

  BinaryOperator *BO = dyn_cast<BinaryOperator>(V);
  if (!BO)
    return false;

This will also decrease the indention.

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1364
@@ +1363,3 @@
+      auto *UI = dyn_cast<Instruction>(U);
+      auto *VI = dyn_cast<Instruction>(V);
+      if (UI && VI && (UI->getParent() == VI->getParent()))
----------------
Is VI loop invariant?  If so, please hoist this out of the loop.  Also, I believe we know V is either a cmp or BO, so we can use a cast<> rather than a dyn_cast<>?


http://reviews.llvm.org/D16978