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

[George Burgess IV via llvm-commits]

(I'm assuming that bryant's point isn't the end of the world for this
patch.)

On Wed, Jan 25, 2017 at 10:39 AM, George Burgess IV via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170125/e0b81b93/attachment.html>