[InstCombine] Do not keep samesign when speculatively executing icmps

[dtcxzyw]

Closes #126974.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Closes #126974.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+5)
• (modified) llvm/test/Transforms/InstCombine/umax-icmp.ll (+24)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 0feb6160b68fb..00a8117f32e70 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -5636,6 +5636,11 @@ Instruction *InstCombinerImpl::foldICmpWithMinMax(Instruction &I,
       return false;
     return std::nullopt;
   };
+  // Remove samesign here since it is illegal to keep it when we speculatively
+  // execute comparisons. For example, `icmp samesign ult umax(X, -46), -32`
+  // cannot be decomposed into `(icmp samesign ult X, -46) or (icmp samesign ult
+  // -46, -32)`. `X` is allowed to be non-negative here.
+  Pred = static_cast<CmpInst::Predicate>(Pred);
   auto CmpXZ = IsCondKnownTrue(simplifyICmpInst(Pred, X, Z, Q));
   auto CmpYZ = IsCondKnownTrue(simplifyICmpInst(Pred, Y, Z, Q));
   if (!CmpXZ.has_value() && !CmpYZ.has_value())
diff --git a/llvm/test/Transforms/InstCombine/umax-icmp.ll b/llvm/test/Transforms/InstCombine/umax-icmp.ll
index b4eea30bfc6af..0c42d26750e4b 100644
--- a/llvm/test/Transforms/InstCombine/umax-icmp.ll
+++ b/llvm/test/Transforms/InstCombine/umax-icmp.ll
@@ -804,4 +804,28 @@ end:
   ret void
 }
 
+define i1 @pr126974(i8 %x) {
+; CHECK-LABEL: @pr126974(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[COND:%.*]] = icmp sgt i8 [[X:%.*]], -2
+; CHECK-NEXT:    br i1 [[COND]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i8 [[X]], -1
+; CHECK-NEXT:    ret i1 [[CMP]]
+; CHECK:       if.else:
+; CHECK-NEXT:    ret i1 false
+;
+entry:
+  %cond = icmp sgt i8 %x, -2
+  br i1 %cond, label %if.then, label %if.else
+
+if.then:
+  %umax = call i8 @llvm.umax.i8(i8 %x, i8 -46)
+  %cmp = icmp samesign ult i8 %umax, -32
+  ret i1 %cmp
+
+if.else:
+  ret i1 false
+}
+
 declare i32 @llvm.umax.i32(i32, i32)

[dtcxzyw]

samesign flag is still needed before this line. We can avoid isKnownNonNegative queries if samesign is set.

[dtcxzyw]

As an alternative, we can pass UseInstrInfo=false to simplifyICmpInst.

[artagnon]

Can you confirm that the bug is in the following line?

bool IsSame = MinMax->getPredicate() == ICmpInst::getStrictPredicate(Pred);

Sounds like we need to use getMatching() here to resolve the bug? Sorry, I'm a bit confused by your fix.

[artagnon]

As an alternative, we can pass UseInstrInfo=false to simplifyICmpInst.

Is the underlying issue simplifyICmpInst though?

[dtcxzyw]

As an alternative, we can pass UseInstrInfo=false to simplifyICmpInst.

Is the underlying issue simplifyICmpInst though?

Yeah. If UseInstrInfo is false, we cannot use poison-generating flags.

[artagnon]

As an alternative, we can pass UseInstrInfo=false to simplifyICmpInst.

Is the underlying issue simplifyICmpInst though?

Yeah. If UseInstrInfo is false, we cannot use poison-generating flags.

Very sorry about this, but I'm still having trouble understanding which exact UseInstrInfo check would resolve the problem. Is the problem simplifyICmpWithMinMax? Does it call simplifyICmpWithConstant, and is the UseInstrInfo check there?

Also, could you kindly add tests for exhaustive signed-unsiged-samesign combinations?

[dtcxzyw]

Is the problem simplifyICmpWithMinMax?

No. We simplify icmp samesign ult X, -46 into false according to the dominating condition icmp sgt i8 %x, -2.

Does it call simplifyICmpWithConstant, and is the UseInstrInfo check there?

I mean we should drop samesign at the beginning of simplifyICmpInst if UseInstrInfo is false.

[dtcxzyw]

As an alternative, we can pass UseInstrInfo=false to simplifyICmpInst.

I prefer the method in this patch. It is still legal to use poison-generating flags in inner expressions.

I mean we should drop samesign at the beginning of simplifyICmpInst if UseInstrInfo is false.

Both simplifyICmpInst and isImpliedCondition APIs don't respect this option.

[nikic]

LGTM

[dtcxzyw]

/cherry-pick 29f3a35

[llvmbot]

/pull-request #127391