[InstCombine] Avoid faulty combines of select-cmp-br

[bjope]

Summary

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.

[spatel]

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

160–168

Please run 'opt -instnamer' or manually add some names to the test. That makes it easier to see what is happening.

[bjope]

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

160–168

Ok, I tried to just follow the pattern with anonymous names as used in the other tests in this file. But I can definitely add some names to my new test. I guess, in particular, that it is hard to see the connection between "label %4" and "<label>:2".

Btw, do you have any thoughts about how to verify this?
The purpose with this test is to verify that the select-cmp-br rewrite won't trigger. So the checks that I've added actually verifies some other instcombine rewrites that happens instead of select-cmp-br.
I could have added CHECK-NOT checks instead, to only verify that we do not get the faulty pattern that was generated before my correction in replacedSelectWithOperand. The drawback with such a solution is that there are lots of ways to do a faulty optimization and I can not CHECK-NOT all of them. So I preferred to have CHECKs that validates that the transformed code is correct (to a certain degree, similar to other tests in this file...).

[spatel]

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

160–168

Ah, sorry - I didn't even look at the rest of the file. Please update past rL296673. Hopefully, that takes care of the other tests. :)

I definitely agree that you want to CHECK for correctness rather than CHECK-NOT for bugs. We have a script that makes this easy at utils/update_test_checks.py, so you can use that (that's what I used for the other tests).

I'm not sure if you can write a test for this that doesn't excite any other transforms, but that's ok as long we're checking the complete output to make sure that it is correct.

[bjope]

Updated the test case (as suggested by spatel).

• Rebased to later revision on trunc
• Removed the irrelevant add from test6
• Generated names using opt --instnamer
• Generated checks using utils/update_test_checks.py

[spatel]

LGTM.

lib/Transforms/InstCombine/InstCombineCompares.cpp

 /// 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) {
     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;
     }
   }
   return false;
 }
 

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
 }
+
+; 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
+}