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

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
spatel added reviewers: aqjune, nikic, lebedev.ri.
Herald added subscribers: hiraditya, kristof.beyls, mcrosier.
spatel requested review of this revision.
Herald added a project: LLVM.

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 <https://reviews.llvm.org/rG5af8bacc940243038478da1c92c3481cbdfcece3> ( D104661 <https://reviews.llvm.org/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.


https://reviews.llvm.org/D105298

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


Index: llvm/test/Transforms/InstCombine/icmp.ll
===================================================================
--- llvm/test/Transforms/InstCombine/icmp.ll
+++ llvm/test/Transforms/InstCombine/icmp.ll
@@ -3935,7 +3935,7 @@
 
 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_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
Index: llvm/lib/Analysis/InstructionSimplify.cpp
===================================================================
--- llvm/lib/Analysis/InstructionSimplify.cpp
+++ llvm/lib/Analysis/InstructionSimplify.cpp
@@ -499,6 +499,11 @@
   if (TCmp == FCmp)
     return TCmp;
 
+  // It is not safe to propagate poison to the compare unless both arms of the
+  // select were poison.
+  if (isa<PoisonValue>(TCmp) || isa<PoisonValue>(FCmp))
+    return nullptr;
+
   // 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())


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D105298.355940.patch
Type: text/x-patch
Size: 1460 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210701/88b6871a/attachment.bin>