[PATCH] D68659: [MemorySSA] Make the use of moveAllAfterMergeBlocks consistent.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

Thanks for this!

LGTM with a few nits



================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:1185
+  auto *Defs = MSSA->getWritableBlockDefs(From);
+  if (Defs && Defs->begin() != Defs->end())
+    if (auto *Phi = dyn_cast<MemoryPhi>(&*Defs->begin()))
----------------
nit: `!Defs->empty()`?

(Though I'm surprised this check is required -- shouldn't we be removing entries from the map as they become empty?)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:1186
+  if (Defs && Defs->begin() != Defs->end())
+    if (auto *Phi = dyn_cast<MemoryPhi>(&*Defs->begin()))
+      tryRemoveTrivialPhi(Phi);
----------------
If I'm reading the description properly, this function assumes that everything in `From` is now in `To`, no?

If so, `cast<MemoryPhi>()` seems more appropriate here, since the only thing that can remain is a single Phi


================
Comment at: test/Analysis/MemorySSA/pr43569.ll:1
+; RUN: opt -pgo-kind=pgo-instr-gen-pipeline -aa-pipeline=default -passes="default<O3>" -enable-nontrivial-unswitch -S < %s | FileCheck %s
+
----------------
; REQUIRES: asserts?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68659/new/

https://reviews.llvm.org/D68659