[PATCH] D66033: [MemorySSA] Rename uses when inserting memory uses.

[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.

lgtm with a few nits. Thanks!



================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:233
+
+  // Above rule 1. is broken by having unreachable blocks, where the unnecessary
+  // Phis were optimized out. Adding the Use may re-insert those Phis. Hence,
----------------
nit: can we please add a bit to the comment above instead of immediately contradicting it? e.g.

"In cases without unreachable blocks, because uses do not create new may-defs, there are only two cases: [...]"

"In cases with unreachable blocks, where the unnecessary Phis were optimized out, adding the Use [...]"


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:237
+  // were added, rename all uses if we are asked. This only happens in the
+  // presence of unreachable blocks.
+  if (RenameUses && InsertedPHIs.size()) {
----------------
Is there a cheap way to `assert` that no renaming needs to be done if `RenameUses == false`?

Even if it might fail to catch some cases, if we can do something as simple as `if (!RenameUses && !InsertedPHIs.empty()) assert(TheBBContainingMUHasNoDefsExceptForThisPhi);` that can catch trivial misuses might be valuable.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:253
+      for (auto &MP : InsertedPHIs) {
+        MemoryPhi *Phi = dyn_cast_or_null<MemoryPhi>(MP);
+        if (Phi)
----------------
s/dyn_cast/cast/ please

also please sink into the `if` condition


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:1103
   else
-    insertUse(cast<MemoryUse>(What));
+    insertUse(cast<MemoryUse>(What), true);
 
----------------
nit: `/*RenameUses=*/` 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66033