[PATCH] D51398: [InstCombine] Fold (min/max ~X, Y) -> ~(max/min X, ~Y) when Y is freely invertible

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a subscriber: davidxl.
spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2926
     if (SelectPatternResult::isMinOrMax(SPF)) {
-      Value *X;
-      if (match(RHS, m_Not(m_Value(X))))
-        std::swap(RHS, LHS);
-
-      if (match(LHS, m_Not(m_Value(X)))) {
+      // It's possible we get here before the not has been simplied, so make
+      // sure the input to the not isn't freely invertible.
----------------
simplied -> simplified


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2934
+
+      // It's possible we get here before the not has been simplied, so make
+      // sure the input to the not isn't freely invertible.
----------------
simplied -> simplified


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:1821-1822
 
       // MAX(~a, ~b) -> ~MIN(a, b)
       // MIN(~a, ~b) -> ~MAX(a, b)
       Value *A, *B;
----------------
Update this comment to match the new logic.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:1842
+
+
       if (Instruction *I = factorizeMinMaxTree(SPF, LHS, RHS, Builder))
----------------
Extra whitespace?


================
Comment at: test/Transforms/InstCombine/select.ll:1332
 
 ; max(max(~a, -1), -1) --> max(~a, -1)
 
----------------
Update comment for new form.


================
Comment at: test/Transforms/InstCombine/select_meta.ll:197
 
-; The compare should change, but the metadata remains the same because the select operands are not swapped.
+; FIXME: Should we preserve the metadata here when we push the not through?
 define i32 @smin1(i32 %x) {
----------------
cc @davidxl 
This is the same compare that we started with, so I think branch weight metadata should transfer as-is.
If we drop metadata in this patch, it's possible we could see perf regressions, so we should try to get this right.


================
Comment at: test/Transforms/InstCombine/sub.ll:1061
+; Tests for (neg (max ~X, C)) -> ((min X, ~C) + 1). Same for min.
+define i32 @test64(i32 %x) {
+; CHECK-LABEL: @test64(
----------------
spatel wrote:
> As usual, I'd prefer that the tests are committed first with baseline checks.
Given the inf-loop potential, there's even more reason to get these committed before the code patch - don't want to lose those in case of revert.


https://reviews.llvm.org/D51398