[llvm] [MSSAUpdater] Replace recursion with worklist and cap it. (PR #150543)

[Alina Sbirlea via llvm-commits]

================
@@ -230,9 +223,61 @@ MemoryAccess *MemorySSAUpdater::tryRemoveTrivialPhi(MemoryPhi *Phi,
     removeMemoryAccess(Phi);
   }
 
-  // We should only end up recursing in case we replaced something, in which
-  // case, we may have made other Phis trivial.
-  return recursePhi(Same);
+  // Continue traversal in a DFS worklist approach, in case we might find
+  // other trivial Phis.
+  if (!Same)
+    return nullptr;
+
+  TrackingVH<MemoryAccess> Result(Same);
----------------
alinas wrote:

I fully agree with you on this one. The use of TrackingVH has been really baked into MSSA and its updater since the beginning and this is trying to deal with that (e.g.
[MemorySSAUpdater.cpp:68](https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/MemorySSAUpdater.cpp#L68) passes in a VH, and removing phis [MemorySSAUpdater.cpp:1326](https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/MemorySSAUpdater.cpp#L1326) checks if a VH is passed in, to do RAUW). My initial attempts to use a visited list here and drop the VH resulted in asserting where a VH was expected (in [Value.cpp](https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Value.cpp#L1171) ).

I'm happy to keep trying to address this and I'm open to feedback on what I missed when triggering the assert. The updated profile with this change, for the original test that found this issue, does show too much time spent on creating VH here. But it is a step fwd since with this change I can cap the recursion and number of phis processed.


https://github.com/llvm/llvm-project/pull/150543