[InstCombine] fix sub(add(X,Y),umin(Y,Z)) --> add(X,usub.sat(Y,Z))

[Chenbing.Zheng]

Summary

This patch fix bug left in D124503.
We should do
sub(add(X,Z),umin(Y,Z)) --> add(X,usub.sat(Z,Y))
instead of
sub(add(X,Z),umin(Y,Z)) --> add(X,usub.sat(Y,Z)).

I forget to update sub-minmax.ll in D124503,
[[TMP1:%.*]] = call i8 @llvm.usub.sat.i8(i8 [[Y:%.*]], i8 [[Z:%.*]]) and
[[TMP1:%.*]] = call i8 @llvm.usub.sat.i8(i8 [[Z:%.*]], i8 [[Y:%.*]]) can both pass llvm-lit test.

[spatel]

This shows a shortcoming of the auto-generated check lines. We don't guarantee that we have captured the parameters with the displayed names, so y and z were swapped (miscompile), but it still passed testing. We used to capture parameters directly with the script, so there was no risk of this bug, but that changed somewhere along the line to make the script more flexible. But we can still do that with a script option:
400587ba0c8b1453

Please update/rebase.

[spatel]

I accidentally marked as approved (the patch itself looks ok, but we should update the tests here first).

[Chenbing.Zheng]

rebase and update tests

[spatel]

LGTM

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp

if (auto *II = dyn_cast<MinMaxIntrinsic>(Op1)) {
         Intrinsic::ID InvID = getInverseMinMaxIntrinsic(II->getIntrinsicID());
         Value *InvMaxMin = Builder.CreateBinaryIntrinsic(InvID, X, Y);
         return replaceInstUsesWith(I, InvMaxMin);
       }
     }
 
     {
       // sub(add(X,Y),umin(Y,Z)) --> add(X,usub.sat(Y,Z))
-      // sub(add(X,Z),umin(Y,Z)) --> add(X,usub.sat(Y,Z))
+      // sub(add(X,Z),umin(Y,Z)) --> add(X,usub.sat(Z,Y))
       Value *X, *Y, *Z;
-      if (match(Op1, m_OneUse(m_UMin(m_Value(Y), m_Value(Z)))) &&
-          (match(Op0, m_OneUse(m_c_Add(m_Specific(Y), m_Value(X)))) ||
-           match(Op0, m_OneUse(m_c_Add(m_Specific(Z), m_Value(X)))))) {
-        Value *USub =
-            Builder.CreateIntrinsic(Intrinsic::usub_sat, I.getType(), {Y, Z});
-        return BinaryOperator::CreateAdd(X, USub);
+      if (match(Op1, m_OneUse(m_UMin(m_Value(Y), m_Value(Z))))) {
+        if (match(Op0, m_OneUse(m_c_Add(m_Specific(Y), m_Value(X)))))
+          return BinaryOperator::CreateAdd(
+              X, Builder.CreateIntrinsic(Intrinsic::usub_sat, I.getType(),
+                                         {Y, Z}));
+        if (match(Op0, m_OneUse(m_c_Add(m_Specific(Z), m_Value(X)))))
+          return BinaryOperator::CreateAdd(
+              X, Builder.CreateIntrinsic(Intrinsic::usub_sat, I.getType(),
+                                         {Z, Y}));
       }
     }
   }
 
   {
     // If we have a subtraction between some value and a select between
     // said value and something else, sink subtraction into select hands, i.e.:
     //   sub (select %Cond, %TrueVal, %FalseVal), %Op1

llvm/test/Transforms/InstCombine/sub-minmax.ll

;
   %a = add i8 %x, %y
   %m = call i8 @llvm.umin.i8(i8 %x, i8 %y)
   %s = sub i8 %a, %m
   call void @use8(i8 %a)
   call void @use8(i8 %m)
   ret i8 %s
 }
 
-;
-; TODO: sub(add(X,Y),umin(Y,Z)) --> add(X,usubsat(Y,Z))
-;
+; sub(add(X,Y),umin(Y,Z)) --> add(X,usubsat(Y,Z))
 
 define i8 @sub_add_umin(i8 %x, i8 %y, i8 %z) {
 ; CHECK-LABEL: define {{[^@]+}}@sub_add_umin
 ; CHECK-SAME: (i8 [[X:%.*]], i8 [[Y:%.*]], i8 [[Z:%.*]]) {
 ; CHECK-NEXT:    [[TMP1:%.*]] = call i8 @llvm.usub.sat.i8(i8 [[Y]], i8 [[Z]])
 ; CHECK-NEXT:    [[S:%.*]] = add i8 [[TMP1]], [[X]]
 ; CHECK-NEXT:    ret i8 [[S]]
 ;
   %a = add i8 %x, %y
   %m = call i8 @llvm.umin.i8(i8 %y, i8 %z)
   %s = sub i8 %a, %m
   ret i8 %s
 }
 
 define i8 @sub_add_umin_commute_umin(i8 %x, i8 %y, i8 %z) {
 ; CHECK-LABEL: define {{[^@]+}}@sub_add_umin_commute_umin
 ; CHECK-SAME: (i8 [[X:%.*]], i8 [[Y:%.*]], i8 [[Z:%.*]]) {
-; CHECK-NEXT:    [[TMP1:%.*]] = call i8 @llvm.usub.sat.i8(i8 [[Z]], i8 [[Y]])
+; CHECK-NEXT:    [[TMP1:%.*]] = call i8 @llvm.usub.sat.i8(i8 [[Y]], i8 [[Z]])
 ; CHECK-NEXT:    [[S:%.*]] = add i8 [[TMP1]], [[X]]
 ; CHECK-NEXT:    ret i8 [[S]]
 ;
   %a = add i8 %x, %y
   %m = call i8 @llvm.umin.i8(i8 %z, i8 %y)
   %s = sub i8 %a, %m
   ret i8 %s
 }
;
   %m = call i8 @llvm.umin.i8(i8 %y, i8 %z)
   %s = sub i8 %a, %m
   ret i8 %s
 }
 
 define i8 @sub_add_umin_commute_add_umin(i8 %x, i8 %y, i8 %z) {
 ; CHECK-LABEL: define {{[^@]+}}@sub_add_umin_commute_add_umin
 ; CHECK-SAME: (i8 [[X:%.*]], i8 [[Y:%.*]], i8 [[Z:%.*]]) {
-; CHECK-NEXT:    [[TMP1:%.*]] = call i8 @llvm.usub.sat.i8(i8 [[Z]], i8 [[Y]])
+; CHECK-NEXT:    [[TMP1:%.*]] = call i8 @llvm.usub.sat.i8(i8 [[Y]], i8 [[Z]])
 ; CHECK-NEXT:    [[S:%.*]] = add i8 [[TMP1]], [[X]]
 ; CHECK-NEXT:    ret i8 [[S]]
 ;
   %a = add i8 %y, %x
   %m = call i8 @llvm.umin.i8(i8 %z, i8 %y)
   %s = sub i8 %a, %m
   ret i8 %s
 }