[PATCH] D51964: [InstCombine] Fold (xor (min/max X, Y), -1) -> (max/min ~X, ~Y) when X and Y are freely invertible.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2903
+      // If both sides are freely invertible, then we can get rid of the xor
+      // completelely
+      if (IsFreeToInvert(LHS, !LHS->hasNUsesOrMore(3)) &&
----------------
typo


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2908-2910
+        return SelectInst::Create(
+            Builder.CreateICmp(getInverseMinMaxPred(SPF), NotLHS, NotRHS),
+            NotLHS, NotRHS);
----------------
Move createMinMax() from InstCombineSelect.cpp, so we can use it here and above?


================
Comment at: test/Transforms/InstCombine/xor.ll:813-816
+  %a = sub i32 -2, %x
+  %b = icmp sgt i32 %a, -1
+  %c = select i1 %b, i32 %a, i32 0
+  %d = xor i32 %c, -1
----------------
The test should start in canonical min/max form, so we're not depending on other folds to exercise the new one:
  %a = sub i32 -2, %x
  %b = icmp sgt i32 %a, 0
  %c = select i1 %1, i32 %a, i32 0
  %d = xor i32 %c, -1

...and of course, I'd prefer to have the test in place ahead of the patch.


================
Comment at: test/Transforms/InstCombine/xor.ll:827-830
+  %a = sub i32 -2, %x
+  %b = icmp slt i32 %a, 0
+  %c = select i1 %b, i32 %a, i32 -1
+  %d = xor i32 %c, -1
----------------
This doesn't provide much extra coverage - just changing the constant? Better to have a test with a vector type and/or change the sub to an add?


Repository:
  rL LLVM

https://reviews.llvm.org/D51964