[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.

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1246
@@ -1245,1 +1245,3 @@
 
+static bool CanInvertUsesOf(Value *V, InstCombiner &IC) {
+  // handle trivial case.
----------------
Functions should start with a lower case letter.

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1247
@@ +1246,3 @@
+static bool CanInvertUsesOf(Value *V, InstCombiner &IC) {
+  // handle trivial case.
+  if (V->hasOneUse())
----------------
Sentances start with an upper case letter.

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1251-1253
@@ +1250,5 @@
+
+  // bail out early for vector types.
+  if (V->getType()->isVectorTy())
+    return false;
+
----------------
Why?

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1277-1285
@@ +1276,11 @@
+  unsigned Threshold = 0;
+  for (auto *U : V->users()) {
+    if (!U->hasOneUse())
+      return false;
+    if (dyn_castNotVal(U->user_back())) {
+      Users.push_back(U);
+    } else {
+      ++Threshold;
+    }
+  }
+
----------------
Seems like we might do an awfully large amount of work.  How large does `Users` typically get when this xform is profitable?

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1289
@@ +1288,3 @@
+    return false;
+
+  do {
----------------
What stops `Users.size()` from equaling zero?

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1291
@@ +1290,3 @@
+  do {
+    auto *InvertedUse = cast_or_null<Instruction>(Users.pop_back_val());
+    Instruction *Inst = cast<Instruction>(V);
----------------
You are using `cast_or_null` but indiscriminately dereference the result, this seems wrong.

================
Comment at: test/Transforms/InstCombine/not2.ll:2
@@ +1,3 @@
+; RUN: opt < %s -instcombine -S | FileCheck %s
+; CHECK-NOT: xor
+
----------------
This is too broad, please CHECK your tests more thoroughly.


http://reviews.llvm.org/D16978