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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope created this revision.

When InstCombine is optimizing certain select-cmp-br patterns
it replaces the result of the select in uses outside of the
basic block containing the select. This is only legal if the
path from the select to the outside use is disjoint from all
other paths out from the originating basic block.

The problem found was that InstCombiner::replacedSelectWithOperand
did not consider the case when both edges out from the br pointed
to the same label. In that case the paths aren't disjoint and the
transformation is illegal. This patch avoids the faulty rewrites
by verifying that there is a single flow to the successor where
we want to replace uses.


https://reviews.llvm.org/D30455

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


Index: test/Transforms/InstCombine/select-cmp-br.ll
===================================================================
--- test/Transforms/InstCombine/select-cmp-br.ll
+++ test/Transforms/InstCombine/select-cmp-br.ll
@@ -153,3 +153,25 @@
 ; CHECK: or
 ; CHECK-NOT: select
 }
+
+; 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, i1) {
+entry:
+  %2 = select i1 %1, i32 %0, i32 0
+  %3 = icmp eq i32 %2, 0
+  br i1 %3, label %4, label %4
+
+; <label>:2                                      ; preds = %entry, %entry
+  %5 = add i32 %2, 7
+  ret i32 %5
+
+; CHECK-LABEL: @test6(
+; CHECK-LABEL: entry
+; CHECK-NEXT: br
+; CHECK-LABEL: <label>:2
+; CHECK-NEXT: add
+; CHECK-NEXT: select
+; CHECK-NEXT: ret
+}
Index: lib/Transforms/InstCombine/InstCombineCompares.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ 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.90014.patch
Type: text/x-patch
Size: 2557 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170228/31f5e3b5/attachment.bin>