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

[Philip Reames]

First of all, thank you for working on this.  Very much appreciated.

I didn't make it to the actual implementation yet, but wanted to share my comments on interface.  I'll get back to this hopefully today or tomorrow for the implementation.


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:12
@@ +11,3 @@
+// This file exposes an interface to building/using memory SSA to walk memory
+// instructions using a use/def graph
+//
----------------
This comment will need to be expanded at some point.  That can wait until the code is closer to ready though.  

Correction, the comment I wanted was below.  Might make sense to move some of it into the file comment or provide a forward reference.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:30
@@ +29,3 @@
+// Memory SSA class builds an SSA form that links together the
+// loads, stores, and clobbers (atomics, calls, etc) of a program,
+// so they can be walked easily.
----------------
I would phrase this as: links together memory access instructions such as loads, stores, ...

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:91
@@ +90,3 @@
+
+  AccessType getType() const { return Type; }
+  virtual ~MemoryAccess() {}
----------------
I might avoid the term "Type" here since that has a well defined meaning in LLVM.  Using AccessType consistently would be clearer.  

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:107
@@ +106,3 @@
+  // We automatically allocate the right amount of space
+  void addUse(MemoryAccess *Use) { UseList[NumUses++] = Use; }
+  MemoryAccess(AccessType AT, BasicBlock *BB)
----------------
A bounds assertion here would be good.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:129
@@ +128,3 @@
+
+  MemoryAccess *getUseOperand() const { return UseOperand; }
+  void setUseOperand(MemoryAccess *UO) { UseOperand = UO; }
----------------
The term "UseOperand" isn't clear to me here.  Is this the link back to a def or phi?

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:155
@@ +154,3 @@
+
+  unsigned int getDefVersion() { return DefVersion; }
+  void setDefVersion(unsigned int v) { DefVersion = v; }
----------------
I'm not sure we need an explicit def version here.  Doesn't the address of the underlying instruction serve that purpose as well?  (And when that instruction disappears, we need to renumber anyways.)

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:175
@@ +174,3 @@
+public:
+  MemoryPhi(unsigned int DV, BasicBlock *BB)
+      : MemoryAccess(AccessPhi, BB), DefVersion(DV) {}
----------------
Given we know the number of predecessors, we should reserve Args space.

================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:239
@@ +238,3 @@
+  // the nearest dominating clobbering Memory Access (by skipping non-aliasing
+  // def links)
+  MemoryAccess *getClobberingMemoryAccess(Instruction *);
----------------
Hm, I'm tempted to leave this functionality in the caller for the moment.  It's not a key part of what MemorySSA is and could be implemented on top of getMemoryAccess

http://reviews.llvm.org/D7864

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