[PATCH] D30455: [InstCombine] Avoid faulty combines of select-cmp-br

[Bjorn Pettersson via Phabricator via llvm-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL296752: [InstCombine] Avoid faulty combines of select-cmp-br (authored by bjope).

Changed prior to commit:
  https://reviews.llvm.org/D30455?vs=90301&id=90336#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30455

Files:
  llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
  llvm/trunk/test/Transforms/InstCombine/select-cmp-br.ll


Index: llvm/trunk/test/Transforms/InstCombine/select-cmp-br.ll
===================================================================
--- llvm/trunk/test/Transforms/InstCombine/select-cmp-br.ll
+++ llvm/trunk/test/Transforms/InstCombine/select-cmp-br.ll
@@ -242,3 +242,22 @@
   br label %bb
 }
 
+; Negative test. Must not trigger the select-cmp-br combine because the result
+; of the select is used in both flows following the br (the special case where
+; the conditional branch has the same target for both flows).
+define i32 @test6(i32 %arg, i1 %arg1) {
+; CHECK-LABEL: @test6(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br i1 undef, label [[BB:%.*]], label [[BB]]
+; CHECK:       bb:
+; CHECK-NEXT:    [[TMP:%.*]] = select i1 [[ARG1:%.*]], i32 [[ARG:%.*]], i32 0
+; CHECK-NEXT:    ret i32 [[TMP]]
+;
+entry:
+  %tmp = select i1 %arg1, i32 %arg, i32 0
+  %tmp2 = icmp eq i32 %tmp, 0
+  br i1 %tmp2, label %bb, label %bb
+
+bb:                                               ; preds = %entry, %entry
+  ret i32 %tmp
+}
Index: llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
===================================================================
--- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -3950,16 +3950,18 @@
   assert((SIOpd == 1 || SIOpd == 2) && "Invalid select operand!");
   if (isChainSelectCmpBranch(SI) && Icmp->getPredicate() == ICmpInst::ICMP_EQ) {
     BasicBlock *Succ = SI->getParent()->getTerminator()->getSuccessor(1);
-    // The check for the unique predecessor is not the best that can be
+    // The check for the single predecessor is not the best that can be
     // done. But it protects efficiently against cases like when SI's
     // home block has two successors, Succ and Succ1, and Succ1 predecessor
     // of Succ. Then SI can't be replaced by SIOpd because the use that gets
     // replaced can be reached on either path. So the uniqueness check
     // guarantees that the path all uses of SI (outside SI's parent) are on
     // is disjoint from all other paths out of SI. But that information
     // is more expensive to compute, and the trade-off here is in favor
-    // of compile-time.
-    if (Succ->getUniquePredecessor() && dominatesAllUses(SI, Icmp, Succ)) {
+    // of compile-time. It should also be noticed that we check for a single
+    // predecessor and not only uniqueness. This to handle the situation when
+    // Succ and Succ1 points to the same basic block.
+    if (Succ->getSinglePredecessor() && dominatesAllUses(SI, Icmp, Succ)) {
       NumSel++;
       SI->replaceUsesOutsideBlock(SI->getOperand(SIOpd), SI->getParent());
       return true;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D30455.90336.patch
Type: text/x-patch
Size: 2707 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170302/d4b713a3/attachment.bin>