[LoopVersioning] Don't modify the list that we iterate over in addPHINodes

[bjope]

Summary

In LoopVersioning::addPHINodes we need to iterate over all
users for a value "Inst", and if the user is outside of the
VersionedLoop we should replace the use of "Inst" by using
the value "PN" instead.

Replacing the use of "Inst" for a user of "Inst" also means
that Inst->users() is modified. So it is not safe to do the
replace while iterating over Inst->users() as we used to do.
This patch splits the task into two steps. First we iterate
over Inst->users() to find all users that should be updated.
Those users are saved into a local data structure on the stack.
And then, in the second step, we do the actual updates. This
time iterating over the local data structure.

[mzolotukhin]

Looks good to me! (+ a couple of nitpicks inline)

lib/Transforms/Utils/LoopVersioning.cpp

143–144 ↗

(On Diff #147762)

I suggest s/UsersToModify/UsersToUpdate/ and remove the comments as the code is pretty clear and self-explanatory.

145 ↗

(On Diff #147762)

How about renaming variable User to U? It's shorter and the new variable name won't overlap with the class name (which is not an error, but just a minor inconvenience when one searches for the variable name in the file, for instance).

[bjope]

Updated according to suggestions from @mzolotukhin.
Thanks for the review (I'll push this tomorrow when I can observe the build bots).

llvm/trunk/lib/Transforms/Utils/LoopVersioning.cpp

for (auto *Inst : DefsUsedOutside) {
     for (auto I = PHIBlock->begin(); (PN = dyn_cast<PHINode>(I)); ++I) {
       if (PN->getIncomingValue(0) == Inst)
         break;
     }
     // If not create it.
     if (!PN) {
       PN = PHINode::Create(Inst->getType(), 2, Inst->getName() + ".lver",
                            &PHIBlock->front());
-      for (auto *User : Inst->users())
-        if (!VersionedLoop->contains(cast<Instruction>(User)->getParent()))
-          User->replaceUsesOfWith(Inst, PN);
+      SmallVector<User*, 8> UsersToUpdate;
+      for (User *U : Inst->users())
+        if (!VersionedLoop->contains(cast<Instruction>(U)->getParent()))
+          UsersToUpdate.push_back(U);
+      for (User *U : UsersToUpdate)
+        U->replaceUsesOfWith(Inst, PN);
       PN->addIncoming(Inst, VersionedLoop->getExitingBlock());
     }
   }
 
   // Then for each PHI add the operand for the edge from the cloned loop.
   for (auto I = PHIBlock->begin(); (PN = dyn_cast<PHINode>(I)); ++I) {
     assert(PN->getNumOperands() == 1 &&
            "Exit block should only have on predecessor");

llvm/trunk/test/Transforms/LoopVersioning/add-phi-update-users.ll

+; RUN: opt < %s -loop-versioning -S -o - | FileCheck %s
+
+; This test case used to end like this:
+;
+;    Instruction does not dominate all uses!
+;      %t2 = load i16, i16* @b, align 1, !tbaa !2, !alias.scope !6
+;      %tobool = icmp eq i16 %t2, 0
+;    LLVM ERROR: Broken function found, compilation aborted!
+;
+; due to a fault where we did not replace the use of %t2 in the icmp in
+; for.end, when adding a new PHI node for the versioned loops based on the
+; loop-defined values used outside of the loop.
+;
+; Verify that the code compiles, that we get a versioned loop, and that the
+; uses of %t2 in for.end and if.then are updated to use the value from the
+; added phi node.
+
+; CHECK:       define void @f1
+; CHECK:       for.end:
+; CHECK-NEXT:    %t2.lver = phi i16 [ %t2, %for.body ], [ %t2.lver.orig, %for.body.lver.orig ]
+; CHECK-NEXT:    %tobool = icmp eq i16 %t2.lver, 0
+; CHECK:       if.then:
+; CHECK-NEXT:    store i16 %t2.lver
+
+@a = dso_local global i16 0, align 1
+@b = dso_local global i16 0, align 1
+@c = dso_local global i16* null, align 1
+
+define void @f1() {
+entry:
+  %t0 = load i16*, i16** @c, align 1
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond.backedge, %entry
+  br label %for.body
+
+for.body:                                         ; preds = %for.cond, %for.body
+  %t1 = phi i64 [ 0, %for.cond ], [ %inc, %for.body ]
+  %t2 = load i16, i16* @b, align 1, !tbaa !2
+  store i16 %t2, i16* %t0, align 1, !tbaa !2
+  %inc = add nuw nsw i64 %t1, 1
+  %cmp = icmp ult i64 %inc, 3
+  br i1 %cmp, label %for.body, label %for.end
+
+for.end:                                          ; preds = %for.body
+  %tobool = icmp eq i16 %t2, 0
+  br i1 %tobool, label %for.cond.backedge, label %if.then
+
+for.cond.backedge:                                ; preds = %for.end, %if.then
+  br label %for.cond
+
+if.then:                                          ; preds = %for.end
+  store i16 %t2, i16* @a, align 1, !tbaa !2
+  br label %for.cond.backedge
+}
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"wchar_size", i32 1}
+!1 = !{!"clang version 7.0.0"}
+!2 = !{!3, !3, i64 0}
+!3 = !{!"long long", !4, i64 0}
+!4 = !{!"omnipotent char", !5, i64 0}
+!5 = !{!"Simple C/C++ TBAA"}