[InstCombine] Remove a couple of asserts based on incorrect assumptions

[mgrang]

Summary

These asserts are based on the assumption that the order of true/false operands in a select and those in the compare would always be the same.
This fixes PR39595.

[mgrang]

Consider this case:

%9 = icmp ugt i32 %5, %8
%10 = select i1 %9, i32 %8, i32 %5, !prof !2

Here the order of true/false operands of the select and the compare are different. This breaks the checks (X == SI.getTrueValue() && Y == SI.getFalseValue()) and (X == SI.getFalseValue() && Y == SI.getTrueValue())

[craig.topper]

Those asserts are validating the metadata correctness. Deleting them is hiding a bug I think.

test/Transforms/InstCombine/select-pr39595.ll

10 ↗

(On Diff #173452)

I dont' think most of the attributes on this function are necessary to show the bug.

19 ↗

(On Diff #173452)

Can we hit this bug without these loads?

31 ↗

(On Diff #173452)

This function_entry_count looks unnecessary to demonstrate the issue.

[mgrang]

Simplified unit test.

[mgrang]

@craig.topper Could you please review this? This is a blocker for our internal release.

[craig.topper]

Transforms/InstCombine/select-pr39595.ll

9 ↗

(On Diff #173567)

I think the branch metadata is incorrect here. It should have been swapped. The original operands were "not %y for true" and "not %x" for false with the false case occuring more often meaning not %x is the most often case. Now the operands are %x for true and %y for false, but the branch metadata is the same so now it says %y occurs more often.

[craig.topper]

Transforms/InstCombine/InstCombineSelect.cpp

1823 ↗

(On Diff #173567)

I think "Swapped" in the if should be replace with the condition from the assert "X == SI.getFalseValue() && Y == SI.getTrueValue()". Which is what I had in the original patch that went up for review, but was asked to just use Swapped. Which was wrong I guess.

[mgrang]

Addressed comments.

[craig.topper]

LGTM

llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp

if (SelectPatternResult::isMinOrMax(SPF)) {
         Value *NewCast = Builder.CreateCast(CastOp, NewSI, SelType);
         return replaceInstUsesWith(SI, NewCast);
       }
 
       // MAX(~a, ~b) -> ~MIN(a, b)
       // MAX(~a, C)  -> ~MIN(a, ~C)
       // MIN(~a, ~b) -> ~MAX(a, b)
       // MIN(~a, C)  -> ~MAX(a, ~C)
-      auto moveNotAfterMinMax = [&](Value *X, Value *Y,
-                                    bool Swapped) -> Instruction * {
+      auto moveNotAfterMinMax = [&](Value *X, Value *Y) -> Instruction * {
         Value *A;
         if (match(X, m_Not(m_Value(A))) && !X->hasNUsesOrMore(3) &&
             !IsFreeToInvert(A, A->hasOneUse()) &&
             // Passing false to only consider m_Not and constants.
             IsFreeToInvert(Y, false)) {
           Value *B = Builder.CreateNot(Y);
           Value *NewMinMax = createMinMax(Builder, getInverseMinMaxFlavor(SPF),
                                           A, B);
           // Copy the profile metadata.
           if (MDNode *MD = SI.getMetadata(LLVMContext::MD_prof)) {
             cast<SelectInst>(NewMinMax)->setMetadata(LLVMContext::MD_prof, MD);
             // Swap the metadata if the operands are swapped.
-            if (Swapped) {
-              assert(X == SI.getFalseValue() && Y == SI.getTrueValue() &&
-                     "Unexpected operands.");
+            if (X == SI.getFalseValue() && Y == SI.getTrueValue())
               cast<SelectInst>(NewMinMax)->swapProfMetadata();
-            } else {
-              assert(X == SI.getTrueValue() && Y == SI.getFalseValue() &&
-                     "Unexpected operands.");
-            }
           }
 
           return BinaryOperator::CreateNot(NewMinMax);
         }
 
         return nullptr;
       };
 
-      if (Instruction *I = moveNotAfterMinMax(LHS, RHS, /*Swapped*/false))
+      if (Instruction *I = moveNotAfterMinMax(LHS, RHS))
         return I;
-      if (Instruction *I = moveNotAfterMinMax(RHS, LHS, /*Swapped*/true))
+      if (Instruction *I = moveNotAfterMinMax(RHS, LHS))
         return I;
 
       if (Instruction *I = factorizeMinMaxTree(SPF, LHS, RHS, Builder))
         return I;
     }
   }
 
   // See if we can fold the select into a phi node if the condition is a select.

llvm/trunk/test/Transforms/InstCombine/select-pr39595.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+define i32 @foo(i32 %x, i32 %y) {
+; CHECK-LABEL: foo
+; CHECK:      [[TMP1:%.*]] = icmp ult i32 %y, %x
+; CHECK-NEXT: [[TMP2:%.*]] = select i1 [[TMP1]], i32 %x, i32 %y, !prof ![[$MD0:[0-9]+]]
+; CHECK-NEXT: [[TMP3:%.*]] = xor i32 [[TMP2]], -1
+; CHECK-NEXT: ret i32 [[TMP3:%.*]]
+; CHECK-DAG:  !0 = !{!"branch_weights", i32 6, i32 1}
+
+  %1 = xor i32 %x, -1
+  %2 = xor i32 %y, -1
+  %3 = icmp ugt i32 %1, %2
+  %4 = select i1 %3, i32 %2, i32 %1, !prof !1
+  ret i32 %4
+}
+
+!1 = !{!"branch_weights", i32 1, i32 6}