[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

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

19

Can we hit this bug without these loads?

31

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

lib/Transforms/InstCombine/InstCombineSelect.cpp

if (SelectPatternResult::isMinOrMax(SPF)) {
             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 (Swapped)
               cast<SelectInst>(NewMinMax)->swapProfMetadata();
-            } else {
-              assert(X == SI.getTrueValue() && Y == SI.getFalseValue() &&
-                     "Unexpected operands.");
-            }
           }
 
           return BinaryOperator::CreateNot(NewMinMax);
         }
 
         return nullptr;
       };
 

test/Transforms/InstCombine/select-pr39595.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+%0 = type { %1 }
+%1 = type { %2* }
+%2 = type { i32, i32* }
+
+$foo = comdat any
+
+; Function Attrs: inlinehint norecurse
+define linkonce_odr dso_local i32 @foo(%0* dereferenceable(4)) local_unnamed_addr comdat !prof !1 {
+; CHECK-LABEL: foo
+; CHECK:      [[TMP1:%.*]] = load i32*, i32** inttoptr (i64 8 to i32**), align 8
+; CHECK-NEXT: [[TMP2:%.*]] = load i32, i32* [[TMP1]], align 4
+; CHECK-NEXT: [[TMP3:%.*]] = load i32, i32* inttoptr (i64 4 to i32*), align 4
+; CHECK-NEXT: [[TMP4:%.*]] = icmp ugt i32 [[TMP2]], [[TMP3]]
+; CHECK-NEXT: [[TMP5:%.*]] = select i1 [[TMP4]], i32 [[TMP2]], i32 [[TMP3]], !prof ![[$MD1:[0-9]+]]
+; CHECK-DAG: ![[$MD1]] = !{!"branch_weights", i32 1, i32 6}
+
+  %2 = getelementptr inbounds %2, %2* null, i32 0, i32 1
+  %3 = load i32*, i32** %2, align 4
+  %4 = load i32, i32* %3, align 4
+  %5 = xor i32 %4, -1
+  %6 = getelementptr inbounds i32, i32* null, i32 1
+  %7 = load i32, i32* %6, align 4
+  %8 = xor i32 %7, -1
+  %9 = icmp ugt i32 %5, %8
+  %10 = select i1 %9, i32 %8, i32 %5, !prof !2
+  ret i32 %10
+}
+
+!1 = !{!"function_entry_count", i64 1}
+!2 = !{!"branch_weights", i32 1, i32 6}