[InstCombine] Preserve range information for Op0 -nsw umin(X, Op0) --> usub.sat(Op0, X)

[dtcxzyw]

This implements the first part discussed in #168880.
Alive2: https://alive2.llvm.org/ce/z/CXAcff

For umin(X, Op0) - Op0 we can also preserve the information implied by the nsw flag. But it is a bit more complicated and doesn't benefit real-world cases (See dtcxzyw/llvm-opt-benchmark#3123 and dtcxzyw/llvm-opt-benchmark#3122).

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

This implements the first part discussed in #168880.
Alive2: https://alive2.llvm.org/ce/z/CXAcff

For umin(X, Op0) - Op0 we can also preserve the information implied by the nsw flag. But it is a bit more complicated and doesn't benefit real-world cases (See dtcxzyw/llvm-opt-benchmark#3123 and dtcxzyw/llvm-opt-benchmark#3122).

---

Full diff: https://github.com/llvm/llvm-project/pull/170989.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp (+14-3)
• (modified) llvm/test/Transforms/InstCombine/sub-minmax.ll (+22)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
index 9bee523c7b7e5..e98702cff66a9 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
@@ -2857,9 +2857,20 @@ Instruction *InstCombinerImpl::visitSub(BinaryOperator &I) {
         I, Builder.CreateIntrinsic(Intrinsic::usub_sat, {Ty}, {X, Op1}));
 
   // Op0 - umin(X, Op0) --> usub.sat(Op0, X)
-  if (match(Op1, m_OneUse(m_c_UMin(m_Value(X), m_Specific(Op0)))))
-    return replaceInstUsesWith(
-        I, Builder.CreateIntrinsic(Intrinsic::usub_sat, {Ty}, {Op0, X}));
+  if (match(Op1, m_OneUse(m_c_UMin(m_Value(X), m_Specific(Op0))))) {
+    Value *USub = Builder.CreateIntrinsic(Intrinsic::usub_sat, {Ty}, {Op0, X});
+    if (auto *USubCall = dyn_cast<CallInst>(USub)) {
+      // Preserve range information implied by the nsw flag.
+      const APInt *C;
+      if (I.hasNoSignedWrap() && match(X, m_NonNegative(C))) {
+        ConstantRange CR = ConstantRange::makeExactNoWrapRegion(
+            Instruction::Sub, *C, OverflowingBinaryOperator::NoSignedWrap);
+        USubCall->addParamAttr(
+            0, Attribute::get(I.getContext(), Attribute::Range, CR));
+      }
+    }
+    return replaceInstUsesWith(I, USub);
+  }
 
   // Op0 - umax(X, Op0) --> 0 - usub.sat(X, Op0)
   if (match(Op1, m_OneUse(m_c_UMax(m_Value(X), m_Specific(Op0))))) {
diff --git a/llvm/test/Transforms/InstCombine/sub-minmax.ll b/llvm/test/Transforms/InstCombine/sub-minmax.ll
index c5af57449bf71..36b487f5570f3 100644
--- a/llvm/test/Transforms/InstCombine/sub-minmax.ll
+++ b/llvm/test/Transforms/InstCombine/sub-minmax.ll
@@ -663,6 +663,28 @@ define i8 @umin_sub_op0_use(i8 %x, i8 %y) {
   ret i8 %r
 }
 
+define i8 @umin_nsw_sub_op1_rhs_nneg_constant(i8 %y) {
+; CHECK-LABEL: define {{[^@]+}}@umin_nsw_sub_op1_rhs_nneg_constant
+; CHECK-SAME: (i8 [[Y:%.*]]) {
+; CHECK-NEXT:    [[R:%.*]] = call i8 @llvm.usub.sat.i8(i8 range(i8 -115, -128) [[Y]], i8 13)
+; CHECK-NEXT:    ret i8 [[R]]
+;
+  %u = call i8 @llvm.umin.i8(i8 %y, i8 13)
+  %r = sub nsw i8 %y, %u
+  ret i8 %r
+}
+
+define i8 @umin_nsw_sub_op1_rhs_neg_constant(i8 %y) {
+; CHECK-LABEL: define {{[^@]+}}@umin_nsw_sub_op1_rhs_neg_constant
+; CHECK-SAME: (i8 [[Y:%.*]]) {
+; CHECK-NEXT:    [[R:%.*]] = call i8 @llvm.usub.sat.i8(i8 [[Y]], i8 -13)
+; CHECK-NEXT:    ret i8 [[R]]
+;
+  %u = call i8 @llvm.umin.i8(i8 %y, i8 -13)
+  %r = sub nsw i8 %y, %u
+  ret i8 %r
+}
+
 ;
 ; sub(add(X,Y), s/umin(X,Y)) --> s/umax(X,Y)
 ; sub(add(X,Y), s/umax(X,Y)) --> s/umin(X,Y)

[dtcxzyw]

Wait. I double-checked the original pattern and found that umin has multiple users. This means we never go through this path. This fold should be implemented in visitICmp.

[ParkHanbum]

The reason this conversion is necessary is that during the process of converting operations like x nsw-umin(x, y) to usub.sat, nsw-related information is lost, resulting in missed optimization opportunities.

In my view, by the time it reaches icmp, it's likely already been converted to usub.sat and the information has been lost, so the possibility of icmp handling this appropriately seems low. What do you think?

[ParkHanbum]

If this patch is merged, I believe I can modify the patch I PR'd to handle it within foldICmpUSubSatOrUAddSatWithConstant in visitICmp. Thank you for this.