[MemorySSA] Don't create phi nodes in fixupDefs() (#156021)

The general flow when inserting MemoryDefs is:

 * Insert the def and set it's defining access (may insert phis)
 * Insert IDF phis
 * Update defining access for defs after the new one (fixupDefs)
 * Rename uses if requested

fixupDefs() uses getPreviousDef() which can create new MemoryPHIs, but
for which we're not going to insert IDF phis, so the required dominance
property may not hold.

I believe this is a leftover from a time before the "Insert IDF phis"
step existed. Now that step should already ensure that all necessary
MemoryPhis have been inserted, and we only need to update them.

The fixupDefs() implementation was also returning after updating a
single access, which is not right.

Fixes #47875.
Fixes #117157.
Fixes #152998.
Fixes #155161.
Fixes #155184.

Author: nikic

llvm/lib/Analysis/MemorySSAUpdater.cpp

@@ -411,17 +411,11 @@ void MemorySSAUpdater::insertDef(MemoryDef *MD, bool RenameUses) {
     FixupList.push_back(MD);
   }
 
-  // Remember the index where we stopped inserting new phis above, since the
-  // fixupDefs call in the loop below may insert more, that are already minimal.
+  // Update defining access of following defs.
   unsigned NewPhiIndexEnd = InsertedPHIs.size();
-
-  while (!FixupList.empty()) {
-    unsigned StartingPHISize = InsertedPHIs.size();
-    fixupDefs(FixupList);
-    FixupList.clear();
-    // Put any new phis on the fixup list, and process them
-    FixupList.append(InsertedPHIs.begin() + StartingPHISize, InsertedPHIs.end());
-  }
+  fixupDefs(FixupList);
+  assert(NewPhiIndexEnd == InsertedPHIs.size() &&
+         "Should not insert new phis during fixupDefs()");
 
   // Optimize potentially non-minimal phis added in this method.
   unsigned NewPhiSize = NewPhiIndexEnd - NewPhiIndex;
@@ -504,11 +498,8 @@ void MemorySSAUpdater::fixupDefs(const SmallVectorImpl<WeakVH> &Vars) {
         assert(MSSA->dominates(NewDef, FirstDef) &&
                "Should have dominated the new access");
 
-        // This may insert new phi nodes, because we are not guaranteed the
-        // block we are processing has a single pred, and depending where the
-        // store was inserted, it may require phi nodes below it.
-        cast<MemoryDef>(FirstDef)->setDefiningAccess(getPreviousDef(FirstDef));
-        return;
+        cast<MemoryDef>(FirstDef)->setDefiningAccess(NewDef);
+        continue;
       }
       // We didn't find a def, so we must continue.
       for (const auto *S : successors(FixupBlock)) {

llvm/test/Transforms/LICM/pr117157.ll

@@ -0,0 +1,80 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=licm -verify-memoryssa < %s | FileCheck %s
+
+define void @test(ptr %p) {
+; CHECK-LABEL: define void @test(
+; CHECK-SAME: ptr [[P:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    br label %[[LOOP0:.*]]
+; CHECK:       [[LOOP0]]:
+; CHECK-NEXT:    br label %[[LOOP1:.*]]
+; CHECK:       [[LOOP1]]:
+; CHECK-NEXT:    [[DEC10:%.*]] = phi i64 [ 0, %[[LOOP0]] ], [ 1, %[[LOOP1]] ]
+; CHECK-NEXT:    br i1 false, label %[[LOOP1_EXIT:.*]], label %[[LOOP1]]
+; CHECK:       [[LOOP1_EXIT]]:
+; CHECK-NEXT:    [[DEC10_LCSSA:%.*]] = phi i64 [ [[DEC10]], %[[LOOP1]] ]
+; CHECK-NEXT:    switch i32 0, label %[[LOOP0_LATCH:.*]] [
+; CHECK-NEXT:      i32 0, label %[[LOOP0_LATCH]]
+; CHECK-NEXT:      i32 2, label %[[LOOP3_PREHEADER:.*]]
+; CHECK-NEXT:      i32 1, label %[[LOOP2:.*]]
+; CHECK-NEXT:    ]
+; CHECK:       [[LOOP2]]:
+; CHECK-NEXT:    br i1 false, label %[[LOOP0_LATCH]], label %[[LOOP3_PREHEADER]]
+; CHECK:       [[LOOP3_PREHEADER]]:
+; CHECK-NEXT:    br label %[[LOOP3:.*]]
+; CHECK:       [[LOOP3]]:
+; CHECK-NEXT:    switch i32 0, label %[[EXIT:.*]] [
+; CHECK-NEXT:      i32 0, label %[[LOOP3]]
+; CHECK-NEXT:      i32 1, label %[[LOOP2_LATCH:.*]]
+; CHECK-NEXT:    ]
+; CHECK:       [[LOOP2_LATCH]]:
+; CHECK-NEXT:    br label %[[LOOP2]]
+; CHECK:       [[LOOP0_LATCH]]:
+; CHECK-NEXT:    br label %[[LOOP0]]
+; CHECK:       [[EXIT]]:
+; CHECK-NEXT:    [[DEC10_LCSSA_LCSSA:%.*]] = phi i64 [ [[DEC10_LCSSA]], %[[LOOP3]] ]
+; CHECK-NEXT:    store i64 [[DEC10_LCSSA_LCSSA]], ptr [[P]], align 4
+; CHECK-NEXT:    store i64 1, ptr [[P]], align 4
+; CHECK-NEXT:    ret void
+;
+entry:
+  br label %loop0
+
+loop0:                                            ; preds = %loop0.latch, %entry
+  br label %loop1
+
+loop1:                                            ; preds = %loop1, %loop0
+  %dec10 = phi i64 [ 0, %loop0 ], [ 1, %loop1 ]
+  store i64 %dec10, ptr %p
+  br i1 false, label %loop1.exit, label %loop1
+
+loop1.exit:                                       ; preds = %loop1
+  switch i32 0, label %loop0.latch [
+  i32 0, label %loop0.latch
+  i32 2, label %loop3.preheader
+  i32 1, label %loop2
+  ]
+
+loop2:                                            ; preds = %loop2.latch, %loop1.exit
+  br i1 false, label %loop0.latch, label %loop3.preheader
+
+loop3.preheader:                                  ; preds = %loop1.exit, %loop2
+  br label %loop3
+
+loop3:                                            ; preds = %loop3.preheader, %loop3
+  switch i32 0, label %exit [
+  i32 0, label %loop3
+  i32 1, label %loop2.latch
+  ]
+
+loop2.latch:                                      ; preds = %loop3
+  br label %loop2
+
+loop0.latch:                                      ; preds = %loop2, %loop1.exit, %loop1.exit
+  store i64 0, ptr %p
+  br label %loop0
+
+exit:                                             ; preds = %loop3
+  store i64 1, ptr %p
+  ret void
+}