Fix replacedSelectWithOperand in InstCombiner to handle branch having two same successors.

[yinma]

Summary

The original transformation is

/ entry:
/ %4 = select i1 %3, %C* %0, %C* null
/ %5 = icmp eq %C* %4, null
/ br i1 %5, label %9, label %7
/ ...
/ ; <label>:7 ; preds = %entry
/ %8 = getelementptr inbounds %C* %4, i64 0, i32 0
/ ...
///

transformed to

/
/ %5 = icmp eq %C* %0, null
/ %6 = select i1 %3, i1 %5, i1 true
/ br i1 %6, label %9, label %7
/ ...
/ ; <label>:7 ; preds = %entry
/ %8 = getelementptr inbounds %C* %0, i64 0, i32 0 replace by %0!

However this transformation is only true when branching to two different blocks. We have to make sure this condition true in the code.

[Gerolf]

This looks weird - checking for null and then ignoring it?! Could you check why this IR is generated? I suspect there is either a bug or a missing optimization. In that case I suggest getting that fixed and adding an assertion in the select optimization.

[yinma]

This kind of IR does actually exist. For our case, it happens after Inlining and Jump Threading, Jump Threading merges if else together and generates a branch with true/false target pointing to the same basic block. It is rare case but it happens. So we need test this situation.

lib/Transforms/InstCombine/InstCombineCompares.cpp

 /// major restriction since a NE compare should be 'normalized' to an equal
 /// compare, which usually happens in the combiner and test case
 /// select-cmp-br.ll checks for it.
 bool InstCombiner::replacedSelectWithOperand(SelectInst *SI,
                                              const ICmpInst *Icmp,
                                              const unsigned SIOpd) {
   assert((SIOpd == 1 || SIOpd == 2) && "Invalid select operand!");
   if (isChainSelectCmpBranch(SI) && Icmp->getPredicate() == ICmpInst::ICMP_EQ) {
+    // need to disable for the case branching to same block like
+    // br il %5, label %n, label %n.
+    if (SI->getParent()->getTerminator()->getSuccessor(1) ==
+        SI->getParent()->getTerminator()->getSuccessor(0))
+      return false;
     BasicBlock *Succ = SI->getParent()->getTerminator()->getSuccessor(1);
     // The check for the unique 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

test/Transforms/InstCombine/select-cmp-br.ll

 ; <label>:7                                      ; preds = %entry
   tail call void @foobar()
   br label %4
 ; CHECK-LABEL: @test5(
 ; CHECK-NOT: select
 ; CHECK: or
 ; CHECK-NOT: select
 }
+
+define void @test6(%C*) {
+entry:
+  %1 = getelementptr inbounds %C, %C* %0, i64 0, i32 0, i32 0
+  %m = load i64*, i64** %1, align 8
+  %2 = getelementptr inbounds %C, %C* %0, i64 1, i32 0, i32 0
+  %n = load i64*, i64** %2, align 8
+  %3 = getelementptr inbounds i64, i64* %m, i64 9
+  %4 = bitcast i64* %3 to i64 (%C*)**
+  %5 = load i64 (%C*)*, i64 (%C*)** %4, align 8
+  %6 = icmp eq i64* %m, %n
+  %7 = select i1 %6, %C* %0, %C* null
+  %8 = icmp eq %C* %7, null
+  br i1 %8, label %10, label %10
+
+; <label>:9                                       ; preds = %10, %12
+  ret void
+
+; <label>:10                                      ; preds = %entry
+  %11 = getelementptr inbounds %C, %C* %7, i64 0, i32 0
+  tail call void @bar(%struct.S* %11)
+  br label %9
+
+; CHECK-LABEL: @test6(
+; CHECK: select
+}