[InstCombine] call SimplifyICmpInst with correct context

[jingyue]

Summary

Fixes PR23809. Without passing the context to SimplifyICmpInst, we would
use the assume to prove that the condition feeding the assume is
trivially true (see isValidAssumeForContext in ValueTracking.cpp),
causing the removal of the assume which may be useful for later
optimizations.

[jingyue]

I noticed InstCombine never passes the context to SimplifyICmpInst/SimplifyFCmpInst. I don't see a reason for that AFAIK, but let me know if I'm missing anything. If you think this is a right approach, I'll enhance other call sites to SimplifyICmpInst/SimplifyFCmpInst as well.

[majnemer]

It looks like SimplifyICmpInst calls ComputeSignBit with one of the ICmpInst operands. ComputeSignBit uses safeCxtI to determine which value to use for the context: if a context isn't explicitly specified, the value being interrogated is used as the context.

ISTM that the issue is that the ComputeSignBit interface has no mechanism to say "I don't have a context". One way of fixing this issue at the API boundary is to make the CxtI in ValueTracking an Optional<const Instruction *> and make safeCxtI unconditionally use the contents of the Optional if it is inhabited with a value, even if that value is nullptr.

[jingyue]

IIUC, your proposed solution would essentially prevent SimplifyICmpInst from leveraging any @llvm.assume because computeKnownBitsFromAssume does nothing when Q.CxtI == nullptr.

It sounds an overkill to me, because SimplifyICmpInst does want to leverage @llvm.assumes that don't use the compare instruction. For instance,

b = (a > 1);
__builtin_assume(b);
return a > 0;

it's desirable to replace the second compare a > 0 with true while keeping __builtin_assume(b) just in case we need __builtin_assume(b) in the future.

b = (a > 1);
__builtin_assume(b);
return true;

and the current patch can do this.

What the current patch prevents is SimplifyICmpInst leveraging an assume to simplify its ephemeral values (see [isEphemeralValueOf](http://llvm.org/docs/doxygen/html/ValueTracking_8cpp_source.html#l00372)). That ensures LLVM won't remove __builtin_assume(b) because __builtin_assume(b) is an ephemeral value of b.

[jingyue]

And just to be clear, I agree with you that it's a potential issue that ComputeSignBit is unable to say "I don't have a context". But my concern is, even if we fix that issue, not passing the context instruction to SimplifyICmpInst at all in InstCombine may overkill some useful optimizations.

Btw, I noticed that SimplifyDemandedBits indeed pass UserI as the context instruction (http://llvm.org/docs/doxygen/html/InstCombineSimplifyDemanded_8cpp_source.html#l00075), which seems aligned with the approach in my patch.

[hfinkel]

In D10695#193685, @jingyue wrote:

I noticed InstCombine never passes the context to SimplifyICmpInst/SimplifyFCmpInst. I don't see a reason for that AFAIK, but let me know if I'm missing anything. If you think this is a right approach, I'll enhance other call sites to SimplifyICmpInst/SimplifyFCmpInst as well.

This looks right to me.

[jingyue]

pass correct context to SimplifyFCmpInst

Note that this does not yet change LLVM's behavior because LLVM does not
leverage @llvm.assume to simplify fcmp for now. But anyhow, I believe it's
moving to the right direction.

lib/Transforms/InstCombine/InstCombineCompares.cpp

Instruction *InstCombiner::visitICmpInst(ICmpInst &I) {
   if (Op0Cplxity < Op1Cplxity ||
         (Op0Cplxity == Op1Cplxity &&
          swapMayExposeCSEOpportunities(Op0, Op1))) {
     I.swapOperands();
     std::swap(Op0, Op1);
     Changed = true;
   }
 
-  if (Value *V = SimplifyICmpInst(I.getPredicate(), Op0, Op1, DL, TLI, DT, AC))
+  if (Value *V =
+          SimplifyICmpInst(I.getPredicate(), Op0, Op1, DL, TLI, DT, AC, &I))
     return ReplaceInstUsesWith(I, V);
 
   // comparing -val or val with non-zero is the same as just comparing val
   // ie, abs(val) != 0 -> val != 0
   if (I.getPredicate() == ICmpInst::ICMP_NE && match(Op1, m_Zero()))
   {
     Value *Cond, *SelectTrue, *SelectFalse;
     if (match(Op0, m_Select(m_Value(Cond), m_Value(SelectTrue),
Instruction *InstCombiner::visitFCmpInst(FCmpInst &I) {
   /// before binary operators.
   if (getComplexity(I.getOperand(0)) < getComplexity(I.getOperand(1))) {
     I.swapOperands();
     Changed = true;
   }
 
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
 
-  if (Value *V = SimplifyFCmpInst(I.getPredicate(), Op0, Op1, DL, TLI, DT, AC))
+  if (Value *V =
+          SimplifyFCmpInst(I.getPredicate(), Op0, Op1, DL, TLI, DT, AC, &I))
     return ReplaceInstUsesWith(I, V);
 
   // Simplify 'fcmp pred X, X'
   if (Op0 == Op1) {
     switch (I.getPredicate()) {
     default: llvm_unreachable("Unknown predicate!");
     case FCmpInst::FCMP_UNO:    // True if unordered: isnan(X) | isnan(Y)
     case FCmpInst::FCMP_ULT:    // True if unordered or less than

test/Transforms/InstCombine/pr23809.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+; InstCombine should preserve the call to @llvm.assume.
+define i32 @icmp(i32 %a, i32 %b) {
+; CHECK-LABEL: @icmp(
+  %sum = add i32 %a, %b
+  %1 = icmp sge i32 %sum, 0
+  call void @llvm.assume(i1 %1)
+; CHECK: call void @llvm.assume
+  ret i32 %sum
+}
+
+define float @fcmp(float %a, float %b) {
+; CHECK-LABEL: @fcmp(
+  %sum = fadd float %a, %b
+  %1 = fcmp oge float %sum, 0.0
+  call void @llvm.assume(i1 %1)
+; CHECK: call void @llvm.assume
+  ret float %sum
+}
+
+declare void @llvm.assume(i1)