[PATCH] D29046: MemorySSA: Link all defs together into an intrusive defslist, to make updater easier

[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 after 3 more small nits. Thanks again!



================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:573
+  // can't differentiate the ilist traits based on tags, only on the value
+  // type.  So we can't use iplist for both since iplist's own if you don't
+  // change the traits.  Thus, we use simple_ilist for one, and iplist for the
----------------
dberlin wrote:
> george.burgess.iv wrote:
> > "since iplist's own if you don't change the traits" sounds strange to me
> I tried to rewrite this into something more understandable.
Much better, thanks! (Looks like clang-format broke the comment, though. :P)


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:700
   void verifyUseInDefs(MemoryAccess *, MemoryAccess *) const;
   using AccessMap = DenseMap<const BasicBlock *, std::unique_ptr<AccessList>>;
+  using DefsMap = DenseMap<const BasicBlock *, std::unique_ptr<DefsList>>;
----------------
dberlin wrote:
> george.burgess.iv wrote:
> > 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.
> > 
> > (If it ends up being a big refactor, I'm fine if we do this as a follow-up)
> So, i'm not strongly opposed, but note:
> 
> We currently guarantee two invariants:
> 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)
> 2. If PerBlockDefs has your block, then the defs list for the block is not empty.
> 
> If we put them both together, then obviously we can still maintain invariant 1, but we will break invariant 2.
> 
> Breaking that invariant means things will now have to check that the defs list is not null everywhere.
> 
> Personally, i don't think it's worth it, but happy to hear another opinion :)
> (and obviously, happy to document these invariants)
Then I'm happy with our current approach. Documenting the invariants would be nice, too. :)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1490
     AccessList *Accesses = getOrCreateAccessList(BB);
+    DefsList *Defs = getOrCreateDefsList(BB);
     MemoryPhi *Phi = new MemoryPhi(BB->getContext(), BB, NextID++);
----------------
`insertIntoListsForBlock`?


https://reviews.llvm.org/D29046