[PATCH] D16743: This patch hijacks Danny's MemorySSA, a virtual SSA form for memory. Details on what it looks like are in MemorySSA.h

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:16
@@ +15,3 @@
+// instructions such loads, stores, atomics and calls. Additionally, it does a
+// trivial form of "heap versioning" Every time the memory state changes in the
+// program, we generate a new heap version. It generates MemoryDef/Uses/Phis
----------------
nit: missing period.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:18
@@ +17,3 @@
+// program, we generate a new heap version. It generates MemoryDef/Uses/Phis
+// that are overlayed on top of the existing instructions
+//
----------------
nit: missing period at the end.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:186
@@ +185,3 @@
+
+  // FIXME(gbiv): MemoryUses don't have users. The quick fix for this would be
+  // to make a virtual `getUsers()` with llvm_unreachable in MemoryUse, but I'd
----------------
In the context when user_begin() is called, the memoryAccess must be a def. Given that assumption, you can safely DownCast the memoryAccess to MemoryDef -- the problem is both Def and Phi need to be handled.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:203
@@ +202,3 @@
+  //
+  // ...But then virtual base classes happen (diamond between MemoryAccess and
+  // Def), and those are icky. Not sure if the trait system would need virtual
----------------
I have another suggestion of the hierarchy:

Strictly speaking a MemoryDef  and MemoryUse is not the IsA relationship, but a hasA relationship -- basically a memory def is not a use, but it has any implicit may use to model the may-kill.   With that in mind,  it is probably not a good idea to factor out UseOrDef out as the common ancestor of MemoryDef and MemoryUse. Instead:

1) MemoryDef should have a pointer to a memoryUse instance
2) MemoryDef and Phi Node should have a common ancestor DefOrPhi as drawn above.

Pros: the userList can now be moved into DefOrPhi
Cons: get|setDefininingAccess method now needs to be declared as virtual in MemoryAccess base and overridden in MemoryUse and MemoryDef.  For MemoryDef, it just need to forward the call to the memoryUse it owns.

To avoid virtual call, the memoryAccess can do dispatching via DownCasting depending on memoryAccess type.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:333
@@ +332,3 @@
+  const unsigned ID;
+  UserListType Users;
+};
----------------
Is this redundant? There is one in base class.


http://reviews.llvm.org/D16743