[PATCH] D32414: InstructionSimplify: Make InstSimplify give the same answer about thesame select no matter how it is asked.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin created this revision.
Herald added a subscriber: Prazek.

SimplifySelectInst and SimplifyCmpInst don't agree about simplifying undef select's :

When faced with undef select/phi loops, one chooses the false value, one chooses the true value.

Here's a testcase

  ; ModuleID = 'bugpoint-reduced-simplified.bc'
  source_filename = "bugpoint-output-2e4d63e.bc"
  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
  target triple = "x86_64-unknown-linux-gnu"
  
  ; Function Attrs: nounwind readonly uwtable
  define void @hoge() local_unnamed_addr  align 2 {
  bb:
    %tmp = select i1 undef, i8 0, i8 1
    br label %bb1
  
  bb1:                                              ; preds = %bb1, %bb
    %tmp2 = phi i8 [ %tmp4, %bb1 ], [ %tmp, %bb ]
    %tmp3 = icmp eq i8 %tmp2, 0
    %tmp4 = select i1 %tmp3, i8 1, i8 %tmp2
    br label %bb1
  }

First note: we get caught between threading the select and the phi.
In fact, it recurses repeatedly between thread cmp over select and
thread cmp over phi, and would infinitely loop here if it did not have
a max recursion depth, because it's check for a loop is wrong.
It tries to avoid loops by checking whether RHS dominates the ph, but
it does (because it's a constant). That's not a sufficient check.  It
really needs to check whether the value of the icmp is in a cycle with
the value of the phi[1].  This is unfixable without adding cycle
detection to determine if they are in the same SCC (and so my
alternate patch is to disable threading of cmp over phis. I'm trying
to avoid this).

Let's ignore that for a second and try a simpler fix:

It starts by threading over the phi.  It checks the select at tmp3,
and recurses between the select and the phi until it runs out of
recursion depth (yay!).  It goes back up.

It then says, in thread select:

"   // It didn't simplify.  However if "cmp FV, RHS" is equal to the select

  410 	    // condition then we can replace it with 'false'.  Otherwise give up.
  411 	    if (!isSameCompare(Cond, Pred, FV, RHS))
  412 	      return nullptr;"

We meet all of these tests, and returning the false value for select
i1 %tmp3, i8 1, %tmp2.  It does the same thing for select i1 undef,
i0, i1, threading that select, and returning the *false* value.

The threading also returns the *false* value for the undef select.
Thus, it believes %tmp3 is false.

However, if you call SimplifySelectInst on the select undef, i8 0, i8
1, it returns the *true* value.

This causes newgvn to never converge, because the simplifier is giving
it two answers about the same instruction, depending on how you ask,
and newgvn discovers this discontinuity :(

Note: You don't need newgvn to cause this issue, anything that tried
to fixpoint the simplification would hit the same issue.

For the moment, this makes simplifyCmpInst and simplifySelectInst
consistent about what value they choose.

I have not yet been able to make it otherwise fail due to the
threading/select cycle issue, but i'm sure we'll eventually have to fix that
:(

Note: I have added a testcase to an upcoming patch to NewGVN.

Also note: NewGVN can do this threading transformation properly, and
avoids cycles correctly, so we may want to re-evaluate after we turn
on NewGVN by default.   It proves everything here is dead :)

GVN also discovers this discontinuity, but it has deleted the
instructions by the time it would affect it.

Like I said, happy to not paper over this, but i'd need direction on what we want to do.

I'm actually pretty sure it's safe as long as the simplifier gives the
same answers about the same instructions.


https://reviews.llvm.org/D32414

Files:
  lib/Analysis/InstructionSimplify.cpp


Index: lib/Analysis/InstructionSimplify.cpp
===================================================================
--- lib/Analysis/InstructionSimplify.cpp
+++ lib/Analysis/InstructionSimplify.cpp
@@ -3754,9 +3754,9 @@
     return TrueVal;
 
   if (isa<UndefValue>(CondVal)) {  // select undef, X, Y -> X or Y
-    if (isa<Constant>(TrueVal))
-      return TrueVal;
-    return FalseVal;
+    if (isa<Constant>(FalseVal))
+      return FalseVal;
+    return TrueVal;
   }
   if (isa<UndefValue>(TrueVal))   // select C, undef, X -> X
     return FalseVal;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D32414.96342.patch
Type: text/x-patch
Size: 553 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170424/5b829e99/attachment.bin>