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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 12 inline comments as done.
dberlin added inline comments.


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:114
 
+struct AllAccessTag {};
+struct DefsOnlyTag {};
----------------
george.burgess.iv wrote:
> I dunno how cluttered `llvm::` is with random pass/analysis-specific things, but it may be nice to scope these somehow (e.g. in a MSSAHelpers struct or namespace or ...)
Fixed :)



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


================
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>>;
----------------
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)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1523
     AccessList *Accesses = nullptr;
+    DefsList *Defs = nullptr;
     for (Instruction &I : B) {
----------------
george.burgess.iv wrote:
> Nit: if we're not going to "cache" our old value, I think this can can be sunk to the if.
Fixed. We now cache the old value, so we don't do a lookup for every def.


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1582
   // Phi's always are placed at the front of the block.
   Accesses->push_front(Phi);
+  Defs->push_front(*Phi);
----------------
george.burgess.iv wrote:
> It looks like we have this pattern:
> 
> ```
> void foo(MemoryAccess *MA) {
>   getOrCreateAccessList(MA->getBlock())->add_thing(MA);
>   if (!isa<MemoryUse>(MA)) {
>     getOrCreateDefsList(MA->getBlock())->add_thing(MA);
>   }
> }
> // Where add_thing is one of {insert, push_back, push_front}.
> ```
> 
> a lot. Can we maybe factor that out into a `insertMemoryAccessBefore`/`insertMemoryAccessAfter(AccessList::iterator, MemoryAccess *)` thing that handles the DefList bits for us?
You can't use the AccessList iterator, but we can at least create two helpers


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1613
+      auto *Defs = getOrCreateDefsList(BB);
+      Defs->push_front(*NewAccess);
+   
----------------
george.burgess.iv wrote:
> Looks like we put Phis in this list, too. Is this supposed to push Defs in front of phis?
No, fixed.



================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1642
+    auto DefsPoint =
+        isa<MemoryDef>(InsertPt) ? InsertPt->getDefsIterator() : Defs->end();
+    Defs->insert(DefsPoint, *NewAccess);
----------------
george.burgess.iv wrote:
> Is `Defs->end()` correct here? What if I insert a store before the `MemoryUse` in
> 
> ```
> define void @foo() {
>   %1 = alloca i8
>   ; MemoryUse(liveOnEntry)
>   %2 = load i8, i8* %1
>   ; 1 = MemoryDef(liveOnEntry)
>   store i8 0, i8* %1
> }
> ```
> 
> ?
No, it is not correct.
We have to find the nearest def, above or below, and do before/after.
Writing the helper functions made me realize this as well.

We'll do it the slow way first, then we can make it faster if necessary (we have the block numbering, we could make it note which are defs, and then find the nearest def to the access passed here).
Since this is an update API, i doubt it'll be that slow in practice.


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2384
 }
+
 } // namespace llvm
----------------
george.burgess.iv wrote:
> Is this from clang-format?
Not sure how it got there


================
Comment at: unittests/Transforms/Utils/MemorySSA.cpp:488
 
+#if 0
 // Test out MemorySSA::spliceMemoryAccessAbove.
----------------
george.burgess.iv wrote:
> I'm assuming this is temporary
Yes, it's going away completely as part of the updater update.



https://reviews.llvm.org/D29046