<div dir="ltr">you can ignore the last "please remove" comment; i skimmed over this last night and forgot to remove it after davide caught the dump() call. :)</div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Feb 19, 2017 at 11:44 AM, George Burgess IV via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">george.burgess.iv added a comment.<br>
<br>
handful o'nits to supplement piotr's comments, then this LGTM.<br>
<br>
thanks!<br>
<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAUpdater.h:66<br>
   MemorySSAUpdater(MemorySSA *MSSA) : MSSA(MSSA) {}<br>
-  void insertDef(MemoryDef *Def);<br>
+  // Insert a definition into the MemorySSA IR.  RenameUses will rename any use<br>
+  // below the new def block (and any inserted phis).  RenameUses should be set<br>
----------------<br>
should this be ///?<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>MemorySSA.cpp:1188<br>
<span class="">+      // We already visited this during our renaming, which can happen when<br>
+      // being asked to rename multipleblocks. Figure out the incoming val,<br>
+      // which is the last def<br>
----------------<br>
</span>missing space between "multiple blocks"<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>MemorySSA.cpp:1191<br>
+      if (!Visited.insert(BB).second) {<br>
+        auto *BlockDefs = getWritableBlockDefs(BB);<br>
+        // Incoming value can only change if there is a block def, and in that<br>
----------------<br>
please sink this into the if condition<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>MemorySSA.cpp:1195<br>
+        if (BlockDefs)<br>
+          IncomingVal = &*(BlockDefs->rbegin());<br>
+      } else<br>
----------------<br>
please remove redundant parens<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>MemorySSAUpdater.cpp:299<br>
+     // incoming value.<br>
+    if (isa<MemoryDef>(FirstDef))<br>
+      FirstDef = cast<MemoryDef>(FirstDef)-><wbr>getDefiningAccess();<br>
----------------<br>
please do `if (auto ... = dyn_cast<MemoryDef>(FirstDef))<wbr>` instead<br>
<span class=""><br>
<br>
================<br>
Comment at: unittests/Transforms/Utils/<wbr>MemorySSA.cpp:193<br>
   MSSA.verifyMemorySSA();<br>
+  MSSA.dump();<br>
 }<br>
----------------<br>
</span><span class="">davide wrote:<br>
> IIRC `dump()` is supposed to be used only in a debugger and `print()` should be used instead, but I might be wrong.<br>
</span>please remove<br>
<br>
<br>
<a href="https://reviews.llvm.org/D30154" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D30154</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>