[llvm] 9eb613b - [InstSimplify] do not propagate poison from select arm to icmp user

[Sanjay Patel via llvm-commits]

Author: Sanjay Patel
Date: 2021-07-01T17:40:07-04:00
New Revision: 9eb613b2de3163686b1a4bd1160f15ac56a4b083

URL: https://github.com/llvm/llvm-project/commit/9eb613b2de3163686b1a4bd1160f15ac56a4b083
DIFF: https://github.com/llvm/llvm-project/commit/9eb613b2de3163686b1a4bd1160f15ac56a4b083.diff

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

This is the cause of the miscompile in:
https://llvm.org/PR50944

The problem has likely existed for some time, but it was made visible with:
5af8bacc94024 ( D104661 )
handleOtherCmpSelSimplifications() assumed it can convert select of
constants to bool logic ops, but that does not work with poison.
We had a very similar construct in InstCombine, so the fix here
mimics the fix there.

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.

Differential Revision: https://reviews.llvm.org/D105298

Added: 
    

Modified: 
    llvm/lib/Analysis/InstructionSimplify.cpp
    llvm/test/Transforms/InstCombine/icmp.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
index 2dbd3e0f7ad35..f713d5317b8cf 100644
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
@@ -189,12 +189,15 @@ static Value *handleOtherCmpSelSimplifications(Value *TCmp, Value *FCmp,
   // 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

diff  --git a/llvm/test/Transforms/InstCombine/icmp.ll b/llvm/test/Transforms/InstCombine/icmp.ll
index 95c006e76ccb8..c64f2ac54cd15 100644
--- a/llvm/test/Transforms/InstCombine/icmp.ll
+++ b/llvm/test/Transforms/InstCombine/icmp.ll
@@ -3935,7 +3935,7 @@ bb:
 
 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
@@ -3944,7 +3944,7 @@ define i1 @thread_cmp_over_select_with_poison_trueval(i1 %b) {
 
 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