[InstSimplify] do not propagate poison from select arm to icmp user

[spatel]

Summary

This appears to be the root cause of the miscompile in:
https://llvm.org/PR50944
...and possibly other recently filed bugs.

The problem has likely existed for some time, but it was made visible with:
5af8bacc94024 ( D104661 )
handleOtherCmpSelSimplifications() assumes it can convert select of constants to bool logic ops, and that does not work with poison.

The bug is in instsimplify, but I'm not sure how to reproduce it outside of instcombine. The reason this is visible in instcombine is because we have a hack (FIXME) to bypass simplification of a select when it has an icmp user:
https://github.com/llvm/llvm-project/blob/955f12589940634acc6c9901e8b25534808f691c/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp#L2632

So we get to an unusual case where we are trying to simplify an instruction that has an operand that would have already simplified if we had processed it in normal order.

[efriedma]

I'm skeptical it's correct to explicitly check for PoisonValue here. Does it matter if the value is poison, but not a poison literal? Does it matter if a constant vector has a poison element?

[spatel]

In D105298#2853512, @efriedma wrote:

I'm skeptical it's correct to explicitly check for PoisonValue here. Does it matter if the value is poison, but not a poison literal? Does it matter if a constant vector has a poison element?

Yes, good point. We want to use isGuaranteedNotToBePoison() to be safe IIUC.
I've been trying to find a hole through other folds that would show that difference, but I haven't found one yet...

[nikic]

llvm/lib/Analysis/InstructionSimplify.cpp

508

I think the right place to address this is in handleOtherCmpSelSimplifications. It basically performs the select i1 %a, i1 %b, i1 false -> and i1 %a, %b fold that we have recently disabled in InstCombine. We should guard it by the same impliesPoison() check.

[spatel]

Patch updated:
Use impliesPoison() to match the similar code in InstCombine.

[nikic]

LGTM

llvm/lib/Analysis/InstructionSimplify.cpp

 /// are not equal. This routine handles some logical simplifications.
 static Value *handleOtherCmpSelSimplifications(Value *TCmp, Value *FCmp,
                                                Value *Cond,
                                                const SimplifyQuery &Q,
                                                unsigned MaxRecurse) {
   // If the false value simplified to false, then the result of the compare
   // is equal to "Cond && TCmp".  This also catches the case when the false
   // value simplified to false and the true value to true, returning "Cond".
-  if (match(FCmp, m_Zero()))
+  // Folding select to and/or isn't poison-safe in general; impliesPoison
+  // checks whether folding it does not convert a well-defined value into
+  // poison.
+  if (match(FCmp, m_Zero()) && impliesPoison(TCmp, Cond))
     if (Value *V = SimplifyAndInst(Cond, TCmp, Q, MaxRecurse))
       return V;
   // If the true value simplified to true, then the result of the compare
   // is equal to "Cond || FCmp".
-  if (match(TCmp, m_One()))
+  if (match(TCmp, m_One()) && impliesPoison(FCmp, Cond))
     if (Value *V = SimplifyOrInst(Cond, FCmp, Q, MaxRecurse))
       return V;
   // Finally, if the false value simplified to true and the true value to
   // false, then the result of the compare is equal to "!Cond".
   if (match(FCmp, m_One()) && match(TCmp, m_Zero()))
     if (Value *V = SimplifyXorInst(
             Cond, Constant::getAllOnesValue(Cond->getType()), Q, MaxRecurse))
       return V;
static Value *ThreadCmpOverSelect(CmpInst::Predicate Pred, Value *LHS,
   // If both sides simplified to the same value, then use it as the result of
   // the original comparison.
   if (TCmp == FCmp)
     return TCmp;
 
   // The remaining cases only make sense if the select condition has the same
   // type as the result of the comparison, so bail out if this is not so.
   if (Cond->getType()->isVectorTy() == RHS->getType()->isVectorTy())
     return handleOtherCmpSelSimplifications(TCmp, FCmp, Cond, Q, MaxRecurse);
 
   return nullptr;
 }
 
 /// In the case of a binary operation with an operand that is a PHI instruction,
 /// try to simplify the binop by seeing whether evaluating it on the incoming
 /// phi values yields the same result for every value. If so returns the common
 /// value, otherwise returns null.

llvm/test/Transforms/InstCombine/icmp.ll

bb:
   %i3 = icmp eq i32 %i, %i2
   ret i1 %i3
 }
 
 ; PR50944
 
 define i1 @thread_cmp_over_select_with_poison_trueval(i1 %b) {
 ; CHECK-LABEL: @thread_cmp_over_select_with_poison_trueval(
-; CHECK-NEXT:    ret i1 poison
+; CHECK-NEXT:    ret i1 false
 ;
   %s = select i1 %b, i32 poison, i32 0
   %tobool = icmp ne i32 %s, 0
   ret i1 %tobool
 }
 
 define i1 @thread_cmp_over_select_with_poison_falseval(i1 %b) {
 ; CHECK-LABEL: @thread_cmp_over_select_with_poison_falseval(
-; CHECK-NEXT:    ret i1 poison
+; CHECK-NEXT:    ret i1 true
 ;
   %s = select i1 %b, i32 1, i32 poison
   %tobool = icmp ne i32 %s, 0
   ret i1 %tobool
 }