[PATCH] This patch introduces MemorySSA, a virtual SSA form for memory.Details on what it looks like are in MemorySSA.h

[Philip Reames]

Ok, here's the MemorySSA.cpp parts of the review.  Lots of small style comments, a couple of larger concerns.  At least some of the code duplication would be nice to address before submission, though I wouldn't say it's mandatory as long as it was done quickly thereafter.

I'm uncomfortable with some of the bits inside the CachingMemorySSAWalker routines.  Would you mind submitting the first version of the memory SSA code without them and then reviewing those separately?  There's some very complex logic there that duplicates parts of MDA.  Given how tricky MDA has been, I'm really not comfortable with the code in the walkers as it stands today without a much more careful review


================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:49
@@ +48,3 @@
+
+INITIALIZE_PASS(MemorySSALazy, "memoryssalazy", "Memory SSA", true, true)
+
----------------
Isn't this line redundant with the above?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:79
@@ +78,3 @@
+/// placement algorithm. It is a linear time phi placement algorithm.
+void MemorySSA::determineInsertionPoint(
+    AccessMap &BlockAccesses, const SmallPtrSetImpl<BasicBlock *> &DefBlocks) {
----------------
I'm skipping all the code copied from PMtR.  I'm going to assume it's correct.

p.s. Any chance this could be expressed in a common template used by both places?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:281
@@ +280,3 @@
+  PerBlockAccesses.clear();
+  delete LiveOnEntryDef;
+}
----------------
Can we use smart pointers by chance?  The manual memory management seems unnecessary.  

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:303
@@ +302,3 @@
+  // for every possible instruction in the stream.  Instead, we build
+  // lists, and then throw it out once the use-def form is built.
+  SmallPtrSet<BasicBlock *, 32> DefiningBlocks;
----------------
We no longer throw this out do we?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:321
@@ +320,3 @@
+        InstructionToMemoryAccess.insert(std::make_pair(&I, MU));
+        if (!Accesses) {
+          Accesses = new AccessListType();
----------------
This should be pulled out in a helper lambda or otherwise commoned.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:327
@@ +326,3 @@
+      }
+      if (def) {
+        MemoryDef *MD = new MemoryDef(nullptr, &I, &B, nextID++);
----------------
else if - once you do that, can't this just become:
if (def) {
} else if (use) {
} else {
  llvm_unreachable()
}

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:415
@@ +414,3 @@
+    MemoryDef *MD = new MemoryDef(nullptr, I, I->getParent(), nextID++);
+    InstructionToMemoryAccess.insert(std::make_pair(I, MD));
+
----------------
Don't you need to add these to the per-BB list?  Also, can this code be commoned with the memory SSA construction path?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:427
@@ +426,3 @@
+
+MemoryAccess *MemorySSA::findDominatingDef(Instruction *Use,
+                                           enum InsertionPlace Where) {
----------------
It looks like you're not really using the instruction at all, maybe just pass the basicblock to begin with?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:462
@@ +461,3 @@
+  MemoryAccess *DefiningDef = findDominatingDef(Use, Where);
+  if (!Result.second) {
+    Accesses = Result.first->second;
----------------
Again, can we common this somewhere?  You've got this lazy creation scatter all over the place.

Accesses = ensureBBList(BB)?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:504
@@ +503,3 @@
+      ++AI;
+    return replaceMemoryAccessWithNewAccess(Replacee, Replacer, AI);
+  }
----------------
This seems odd.  Shouldn't we be putting Replacer in the spot Replacee was?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:547
@@ +546,3 @@
+  for (const auto &Arg : MP->args())
+    if (Arg.second == Replacee)
+      if (!DT->dominates(Replacer->getBlock(), Arg.first))
----------------
Did you possible mean a != here?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:560
@@ +559,3 @@
+  // may now be defined in terms of it.
+  bool usedByReplacee = getDefiningAccess(Replacer) == Replacee;
+
----------------
This condition really doesn't feel like a "replacement".  Is it time to reconsider the interface and turn this into an assert?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:571
@@ +570,3 @@
+    if (MemoryPhi *MP = dyn_cast<MemoryPhi>(U)) {
+      for (unsigned i = 0, e = MP->getNumIncomingValues(); i != e; ++i) {
+        if (MP->getIncomingValue(i) == Replacee)
----------------
It might be cleaner to unconditionally insert the new value, then fold single use memory phis. Having a fold single use phi utility might also be useful on the public interface.

Actually, are you expecting an invariant where such phis are always folded?  If so, please update comments to say so.  

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:611
@@ +610,3 @@
+                              "repoint to!");
+    assert(onlySingleValue(MP) && "This phi still points to multiple values, "
+                                  "which means it is still needed");
----------------
If there are no uses, why is it required?  Did you mean:
use_empty || onlySingleValue?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:620
@@ +619,3 @@
+      if (MemoryPhi *MP = dyn_cast<MemoryPhi>(U)) {
+        for (unsigned i = 0, e = MP->getNumIncomingValues(); i != e; ++i) {
+          if (MP->getIncomingValue(i) == MA)
----------------
You've got this code repeated a bit.  Utility function on memoryPhi?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:821
@@ +820,3 @@
+
+void MemorySSAPrinterPass::registerOptions() {
+  OptionRegistry::registerOption<bool, MemorySSAPrinterPass,
----------------
Can't you just use cl opt for this?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:852
@@ +851,3 @@
+
+void MemorySSALazy::releaseMemory() { delete MSSA; }
+
----------------
smart pointer?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:942
@@ +941,3 @@
+  // Don't try to walk past an incomplete phi node during construction
+  if (P->getNumIncomingValues() != P->getNumPreds())
+    return std::make_pair(P, false);
----------------
Er, huh?  How can this happen?  And should it?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1005
@@ +1004,3 @@
+    const LoadInst *QueryLI = cast<LoadInst>(QueryInst);
+    // An unordered load can't be clobbered by an ordered one in any
+    // circumstances.
----------------
Er, what?  This just seems flat out wrong.  Two loads on the same thread to the same location are always ordered regardless of flags.  Ordering doesn't bypass a mustalias...

Unless this is absolutely required, I would request you separate this and review it separately. 

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1025
@@ +1024,3 @@
+    if (MemoryPhi *P = dyn_cast<MemoryPhi>(CurrAccess))
+      return getClobberingMemoryAccess(P, Q);
+    else {
----------------
I might be missing something here, but how is this not an infinite recursion on itself?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1042
@@ +1041,3 @@
+      if (!Q.isCall) {
+        // Okay, well, see if it's a volatile load vs non-volatile load
+        // situation or smoething similar)
----------------
This seems suspicious to me as said above.

More generally, the smarts of this routine deserve to be reviewed independently. 

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1083
@@ +1082,3 @@
+    return StartingAccess;
+  } else if (!isa<CallInst>(I) && !isa<InvokeInst>(I)) {
+    Q.isCall = false;
----------------
Mild preference, change this to:
else if (CallSite CS = ...) {
} else {
}

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1110
@@ +1109,3 @@
+  if (Q.isCall) {
+    for (const auto &C : Q.VisitedCalls)
+      doCacheInsert(C, FinalAccess, Q);
----------------
Does this optimization actually help?  Just curious.

http://reviews.llvm.org/D7864

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/