<div dir="ltr">(I'm assuming that bryant's point isn't the end of the world for this patch.)<div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 25, 2017 at 10:39 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 accepted this revision.<br>
george.burgess.iv added a comment.<br>
This revision is now accepted and ready to land.<br>
<br>
LGTM after 3 more small nits. Thanks again!<br>
<span><br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSA.h:573<br>
+  // can't differentiate the ilist traits based on tags, only on the value<br>
+  // type.  So we can't use iplist for both since iplist's own if you don't<br>
+  // change the traits.  Thus, we use simple_ilist for one, and iplist for the<br>
----------------<br>
</span><span>dberlin wrote:<br>
> george.burgess.iv wrote:<br>
> > "since iplist's own if you don't change the traits" sounds strange to me<br>
> I tried to rewrite this into something more understandable.<br>
</span>Much better, thanks! (Looks like clang-format broke the comment, though. :P)<br>
<span><br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSA.h:700<br>
   void verifyUseInDefs(MemoryAccess *, MemoryAccess *) const;<br>
   using AccessMap = DenseMap<const BasicBlock *, std::unique_ptr<AccessList>>;<br>
+  using DefsMap = DenseMap<const BasicBlock *, std::unique_ptr<DefsList>>;<br>
----------------<br>
</span><span>dberlin wrote:<br>
> george.burgess.iv wrote:<br>
> > Can this just be a map of `const BasicBlock *` to `unique_ptr<struct { AccessList; DefsList; }>` (excuse the not-actual-c++)? Looks like we look both up together a lot, and their lifetimes are sorta-related.<br>
> ><br>
> > (If it ends up being a big refactor, I'm fine if we do this as a follow-up)<br>
> So, i'm not strongly opposed, but note:<br>
><br>
> We currently guarantee two invariants:<br>
> 1. If PerBlockAccesses has your block, then the access list for the block is not empty (and vice versa. If it does not have the block, there are no accesses)<br>
> 2. If PerBlockDefs has your block, then the defs list for the block is not empty.<br>
><br>
> If we put them both together, then obviously we can still maintain invariant 1, but we will break invariant 2.<br>
><br>
> Breaking that invariant means things will now have to check that the defs list is not null everywhere.<br>
><br>
> Personally, i don't think it's worth it, but happy to hear another opinion :)<br>
> (and obviously, happy to document these invariants)<br>
</span>Then I'm happy with our current approach. Documenting the invariants would be nice, too. :)<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA<wbr>.cpp:1490<br>
     AccessList *Accesses = getOrCreateAccessList(BB);<br>
+    DefsList *Defs = getOrCreateDefsList(BB);<br>
     MemoryPhi *Phi = new MemoryPhi(BB->getContext(), BB, NextID++);<br>
----------------<br>
`insertIntoListsForBlock`?<br>
<br>
<br>
<a href="https://reviews.llvm.org/D29046" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2904<wbr>6</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>