[llvm] [InstCombine] Do not apply unsigned sub identities when flipping icmp strictness in `foldICmpBinOp` (PR #219904)

[via llvm-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-transforms

Author: Antonio Frighetto (antoniofrighetto)

<details>
<summary>Changes</summary>

Folding `icmp ult A -nuw B, Op1 -> icmp ule A, Op1` may be unsound for unsigned predicates with negative B, when the subtraction appears as a `or disjoint`, previously matched as `add nuw`. nuw in `A + (-B)` ensures that `A <u B` does not wrap unsigned, though the identity would require proving `A >=u B` for the subtraction not to wrap. As the fold relies on `NoOp0WrapProblem`, which proves the opposite of the latter, simply avoid performing such an optimization.

Fixes: https://github.com/llvm/llvm-project/issues/218540.

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


2 Files Affected:

- (modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+13-6) 
- (modified) llvm/test/Transforms/InstCombine/icmp.ll (+38) 


``````````diff
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 6c486f2cc5e79..72f08b398e45d 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -5395,6 +5395,8 @@ Instruction *InstCombinerImpl::foldICmpBinOp(ICmpInst &I,
              (CmpInst::isUnsigned(Pred) && HasNUW) ||
              (CmpInst::isSigned(Pred) && HasNSW);
     } else if (BO.getOpcode() == Instruction::Or) {
+      // The invariant here is that we are handling m_AddLike instructions,
+      // which can only be a or disjoint, which is equivalent to an add nuw nsw.
       HasNUW = true;
       HasNSW = true;
       return true;
@@ -5474,9 +5476,13 @@ Instruction *InstCombinerImpl::foldICmpBinOp(ICmpInst &I,
       return isMultipleOf(X, C, Q) && isMultipleOf(Y, C, Q);
     };
 
-    // TODO: The subtraction-related identities shown below also hold, but
-    // canonicalization from (X -nuw 1) to (X + -1) means that the combinations
-    // wouldn't happen even if they were implemented.
+    // The subtraction-related identities (A -nuw B) shown below require that
+    // the subtraction does not wrap unsigned (i.e., A >=u B). Canonicalization
+    // from (A -nuw 1) to (A + -1) means that such combinations ought to never
+    // occur, as sub nuw ops should have been canonicalized to add ones. It may
+    // however appear in the form of a or disjoint. Though, or disjoint A, -B
+    // requires proving A <u B, for which the nowrap precondition can never be
+    // satisfied. These are therefore skipped.
     //
     // icmp ult (A - 1), Op1 -> icmp ule A, Op1
     // icmp uge (A - 1), Op1 -> icmp ugt A, Op1
@@ -5489,9 +5495,10 @@ Instruction *InstCombinerImpl::foldICmpBinOp(ICmpInst &I,
     // icmp sgt (A + 1), Op1 -> icmp sge A, Op1
     // icmp ule (A + 1), Op0 -> icmp ult A, Op1
     // icmp ugt (A + 1), Op0 -> icmp uge A, Op1
-    if (A && NoOp0WrapProblem &&
-        ShareCommonDivisor(A, Op1, B,
-                           ICmpInst::isLT(Pred) || ICmpInst::isGE(Pred)))
+    bool IsNegative = ICmpInst::isLT(Pred) || ICmpInst::isGE(Pred);
+    bool IsAddOrSignedPred = !IsNegative || ICmpInst::isSigned(Pred);
+    if (A && NoOp0WrapProblem && IsAddOrSignedPred &&
+        ShareCommonDivisor(A, Op1, B, IsNegative))
       return new ICmpInst(ICmpInst::getFlippedStrictnessPredicate(Pred), A,
                           Op1);
 
diff --git a/llvm/test/Transforms/InstCombine/icmp.ll b/llvm/test/Transforms/InstCombine/icmp.ll
index 61ae46ba08d2d..c8217273ebbde 100644
--- a/llvm/test/Transforms/InstCombine/icmp.ll
+++ b/llvm/test/Transforms/InstCombine/icmp.ll
@@ -2122,6 +2122,44 @@ define i1 @icmp_ule_offset_with_common_divisor(i64 %x, i64 %y) {
   ret i1 %cmp
 }
 
+; Ensure the identity icmp ult (A - B), Op1 to icmp ule A, Op1 does not occur
+; when the or disjoint is matched as an add, as nuw of add does not imply nowrap
+; of the unsigned subtraction.
+; FIXME: This is a miscompilation.
+define i1 @icmp_ult_neg_offset_or_disjoint(i16 %arg) "instcombine-no-verify-fixpoint" {
+; CHECK-LABEL: define i1 @icmp_ult_neg_offset_or_disjoint(
+; CHECK-SAME: i16 [[ARG:%.*]]) #[[ATTR1:[0-9]+]] {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[ZEXT:%.*]] = zext nneg i16 [[ARG]] to i32
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i16 [[ARG]], 0
+; CHECK-NEXT:    call void @use_i32(i32 [[ZEXT]])
+; CHECK-NEXT:    br i1 [[CMP]], label %[[IF:.*]], label %[[ELSE:.*]]
+; CHECK:       [[IF]]:
+; CHECK-NEXT:    [[XOR:%.*]] = xor i16 [[ARG]], -32768
+; CHECK-NEXT:    [[OR:%.*]] = or disjoint i32 [[ZEXT]], -32768
+; CHECK-NEXT:    [[SEXT:%.*]] = sext i16 [[XOR]] to i32
+; CHECK-NEXT:    [[RES:%.*]] = icmp ult i32 [[OR]], [[SEXT]]
+; CHECK-NEXT:    ret i1 [[RES]]
+; CHECK:       [[ELSE]]:
+; CHECK-NEXT:    ret i1 false
+;
+entry:
+  %zext = zext nneg i16 %arg to i32
+  %or = or i32 %zext, -32768
+  %xor = xor i16 %arg, -32768
+  %cmp = icmp ult i16 %arg, 1
+  call void @use_i32(i32 %zext)
+  br i1 %cmp, label %if, label %else
+
+if:
+  %sext = sext i16 %xor to i32
+  %res = icmp ult i32 %or, %sext
+  ret i1 %res
+
+else:
+  ret i1 false
+}
+
 ; TODO: Handle non-power-of-2 divisors
 define i1 @icmp_ule_offset_with_common_non_pow2_divisor(i64 %x, i64 %y) {
 ; CHECK-LABEL: define i1 @icmp_ule_offset_with_common_non_pow2_divisor(

``````````

</details>


https://github.com/llvm/llvm-project/pull/219904