[LoopSimplifyCFG] Fix corner case with duplicating successors

[mkazantsev]

Summary

It fixes a bug that doesn't update Phi inputs of the only live successor that
is in the list of block's successors more than once.

Thanks @uabelho for finding this.

[uabelho]

I've verifed that it solves the problem I reported. I've done a little bit of testing with the patch and didn't see anything obviously broken yet at least.

As for the details of the patch, I'm sure someone else knows better if this is the best way to solve the issue but it looks fine to me.

Thanks!

[anna]

LGTM.

lib/Transforms/Scalar/LoopSimplifyCFG.cpp

for (BasicBlock *BB : FoldCandidates) {
       assert(TheOnlySucc && "Should have one live successor!");
 
       LLVM_DEBUG(dbgs() << "Replacing terminator of " << BB->getName()
                         << " with an unconditional branch to the block "
                         << TheOnlySucc->getName() << "\n");
 
       SmallPtrSet<BasicBlock *, 2> DeadSuccessors;
       // Remove all BB's successors except for the live one.
+      unsigned TheOnlySuccDuplicates = 0;
       for (auto *Succ : successors(BB))
         if (Succ != TheOnlySucc) {
           DeadSuccessors.insert(Succ);
           // If our successor lies in a different loop, we don't want to remove
           // the one-input Phi because it is a LCSSA Phi.
           bool PreserveLCSSAPhi = !L.contains(Succ);
           Succ->removePredecessor(BB, PreserveLCSSAPhi);
-        }
+        } else
+          ++TheOnlySuccDuplicates;
+
+      assert(TheOnlySuccDuplicates > 0 && "Should be!");
+      // If TheOnlySucc was BB's successor more than once, after transform it
+      // will be its successor only once. Remove redundant inputs from
+      // TheOnlySucc's Phis.
+      bool PreserveLCSSAPhi = !L.contains(TheOnlySucc);
+      for (unsigned Dup = 1; Dup < TheOnlySuccDuplicates; ++Dup)
+        TheOnlySucc->removePredecessor(BB, PreserveLCSSAPhi);
 
       IRBuilder<> Builder(BB->getContext());
       Instruction *Term = BB->getTerminator();
       Builder.SetInsertPoint(Term);
       Builder.CreateBr(TheOnlySucc);
       Term->eraseFromParent();
 
       for (auto *DeadSucc : DeadSuccessors)

test/Transforms/LoopSimplifyCFG/phi_with_duplicating_inputs.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; This is currently failing because of bug in LoopSimplifyCFG. It does not update
 ; duplicating Phi inputs properly.
-; XFAIL: *
 ; RUN: opt -S -enable-loop-simplifycfg-term-folding=true -loop-simplifycfg -debug-only=loop-simplifycfg -verify-loop-info -verify-dom-info -verify-loop-lcssa 2>&1 < %s | FileCheck %s
 ; RUN: opt -S -enable-loop-simplifycfg-term-folding=true -passes='require<domtree>,loop(simplify-cfg)' -debug-only=loop-simplifycfg -verify-loop-info -verify-dom-info -verify-loop-lcssa 2>&1 < %s | FileCheck %s
 ; RUN: opt -S -enable-loop-simplifycfg-term-folding=true -loop-simplifycfg -enable-mssa-loop-dependency=true -verify-memoryssa -debug-only=loop-simplifycfg -verify-loop-info -verify-dom-info -verify-loop-lcssa 2>&1 < %s | FileCheck %s
 
 target datalayout = "P40"
 
 @a = external global i16, align 1
 
-; CHECK-LABEL: @f1(
-
 define void @f1(i1 %cond) {
+; CHECK-LABEL: @f1(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_COND:%.*]]
+; CHECK:       for.cond:
+; CHECK-NEXT:    br i1 [[COND:%.*]], label [[IF_THEN:%.*]], label [[FOR_INC:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    [[TMP0:%.*]] = load i16, i16* @a, align 1
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i16 [[TMP0]], 0
+; CHECK-NEXT:    br label [[FOR_INC]]
+; CHECK:       for.inc:
+; CHECK-NEXT:    [[C_1:%.*]] = phi i16 [ 2, [[IF_THEN]] ], [ 1, [[FOR_COND]] ]
+; CHECK-NEXT:    br label [[FOR_COND]]
+;
 entry:
   br label %for.cond
 
 for.cond:
   br i1 %cond, label %if.then, label %for.inc
 
 if.then:
   %0 = load i16, i16* @a, align 1
   %tobool = icmp ne i16 %0, 0
   br i1 %tobool, label %for.inc, label %for.inc
 
 for.inc:
   %c.1 = phi i16 [ 2, %if.then ], [ 2, %if.then ], [ 1, %for.cond ]
   br label %for.cond
 }