[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 added a comment.

Thanks for the patch! The overall approach here seems solid to me.



================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:114
 
+struct AllAccessTag {};
+struct DefsOnlyTag {};
----------------
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 ...)


================
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
----------------
"since iplist's own if you don't change the traits" sounds strange to me


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:689
 
+  
+
----------------
Less newlines, please


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


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1523
     AccessList *Accesses = nullptr;
+    DefsList *Defs = nullptr;
     for (Instruction &I : B) {
----------------
Nit: if we're not going to "cache" our old value, I think this can can be sunk to the if.


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1533
+      if (isa<MemoryDef>(MUD)) {
+        InsertIntoDef |= true;
+        Defs = getOrCreateDefsList(&B);
----------------
`InsertIntoDef = true`


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


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


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1614
+      Defs->push_front(*NewAccess);
+   
+    }
----------------
Please delete this newline (and below)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1642
+    auto DefsPoint =
+        isa<MemoryDef>(InsertPt) ? InsertPt->getDefsIterator() : Defs->end();
+    Defs->insert(DefsPoint, *NewAccess);
----------------
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
}
```

?


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1660
+    auto *Defs = getOrCreateDefsList(InsertPt->getBlock());
+    auto DefsPoint = isa<MemoryDef>(InsertPt) ? ++(InsertPt->getDefsIterator())
+                                              : Defs->end();
----------------
Please remove the extra parens


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1661
+    auto DefsPoint = isa<MemoryDef>(InsertPt) ? ++(InsertPt->getDefsIterator())
+                                              : Defs->end();
+    Defs->insert(DefsPoint, *NewAccess);
----------------
Same "is Defs->end() correct?" comment


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1927
+               "Either we should have a defs list, or we should have no defs");
+    assert((!DL || (DL->size() == ActualDefs.size())) &&
+           "We don't have the same number of defs in the block as on the "
----------------
Please remove the extra parens


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2384
 }
+
 } // namespace llvm
----------------
Is this from clang-format?


================
Comment at: unittests/Transforms/Utils/MemorySSA.cpp:488
 
+#if 0
 // Test out MemorySSA::spliceMemoryAccessAbove.
----------------
I'm assuming this is temporary


https://reviews.llvm.org/D29046