[PATCH] D29047: Introduce a basic MemorySSA updater, that supports insertDef and insertUse operations.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Mostly just nits; I'm assuming that the algorithm in the paper is correct, and I can't immediately see any issues with the logic here.

Thanks for the patch!



================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:746
+  SmallVector<MemoryPhi *, 8> InsertedPHIs;
+  DenseSet<BasicBlock *> VisitedBlocks;
+
----------------
`SmallPtrSet`?


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:761
+  MemoryAccess *tryRemoveTrivialPhi(MemoryPhi *Phi, RangeType &Operands);
+  void fixupDefsAfterUs(const SmallVectorImpl<MemoryAccess *> &);
+};
----------------
Was this a typo? If not, the `AfterUs` seems redundant unless it's from the paper you referenced or something.


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:572
+  // 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 weird to me


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:749
+  MemorySSAUpdater(MemorySSA *MSSA) : MSSA(MSSA) {}
+  void insertDef(MemoryAccess *Def);
+  void insertUse(MemoryAccess *Use);
----------------
Can this just take a Def in the first place (same for the function below, except Use)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2399
+  if (BasicBlock *Pred = BB->getSinglePredecessor()) {
+    Result = getPreviousDefFromEnd(Pred);
+  } else if (VisitedBlocks.count(BB)) {
----------------
Nit: Since we're doing nothing but returning `Result`, can this be converted to returning early? (And below)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2400
+    Result = getPreviousDefFromEnd(Pred);
+  } else if (VisitedBlocks.count(BB)) {
+    // We hit our node again, meaning we had a cycle, we must insert a phi
----------------
`else if (!VisitedBlocks.insert(BB))`? (will be less sketchy if we swap to early returns)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2425
+    if (Result == Phi) {
+      if (!Phi) {
+        Phi = MSSA->createMemoryPhi(BB);
----------------
Useless braces


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2619
+    // Put any new phis on the fixup list, and process them
+    for (unsigned i = InsertedPHIs.size() - StartingPHISize;
+         i < InsertedPHIs.size(); ++i)
----------------
`FixupList.append(InsertedPhis.end() - StartingPHISize, InsertedPhis.end());`?


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2437
+
+// This starts at the memory access
+MemoryAccess *MemorySSAUpdater::getPreviousDef(MemoryAccess *MA) {
----------------
Incomplete comment? (And below * 2)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2475
+// This starts at the end of block
+MemoryAccess *MemorySSAUpdater::getPreviousDefFromEnd(BasicBlock *BB) {
+  auto *Defs = MSSA->getWritableBlockDefs(BB);
----------------
Naming nit: `MemorySSAUpdater::getLastDefIn`?


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2559
+// A brief description of the algorithm:
+// First, we generate figure out what should define the new def, using the ssa
+// construction algorithm.
----------------
"we generate figure out"?


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2650
+      auto *Defs = MSSA->getWritableBlockDefs(FixupBlock);
+      if (Defs) {
+        auto *FirstDef = &*Defs->begin();
----------------
if (auto *Defs = ...)


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2655
+               "Should have already handled phi nodes!");
+        // We are now this def's defining access,make sure we actually dominate
+        // it
----------------
"access, make"


================
Comment at: unittests/Transforms/Utils/MemorySSA.cpp:277
+TEST_F(MemorySSATest, MoveAStoreUpdater) {
+#if 0
+  // We create a diamond where there is a in the entry, a store on one side, and
----------------
Temporary (and below)?


https://reviews.llvm.org/D29047