<div dir="ltr">Thanks so much for taking a look at what i've got so far!<div><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 25, 2015 at 10:01 AM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">First of all, thank you for working on this.  Very much appreciated.<br>
<br>
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.<br></blockquote><div><br></div><div>Okay. Some of it is solid, some of it less so, so other people thinking about it and looking at it are definitely wanted :)</div><div><br></div><div>Let me just respond to a few questions (i'll clean up and integrate your other suggestions directly today)================<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:129<br>
@@ +128,3 @@<br>
+<br>
+  MemoryAccess *getUseOperand() const { return UseOperand; }<br>
+  void setUseOperand(MemoryAccess *UO) { UseOperand = UO; }<br>
----------------<br>
The term "UseOperand" isn't clear to me here.  Is this the link back to a def or phi?<br>
<br></blockquote><div>Yes</div><div><br></div><div>Suggestions for better name welcome</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:155<br>
@@ +154,3 @@<br>
+<br>
+  unsigned int getDefVersion() { return DefVersion; }<br>
+  void setDefVersion(unsigned int v) { DefVersion = v; }<br>
----------------<br>
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.)<br></blockquote><div><br></div><div>So, you are right that this is entirely convenience.</div><div><br></div><div>Truthfully, i wanted to be able to index them easily in vectors (without going through and assigning ids), and print them out nicely without having to  use something like a slottracker.</div><div><br></div><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:175<br>
@@ +174,3 @@<br>
+public:<br>
+  MemoryPhi(unsigned int DV, BasicBlock *BB)<br>
+      : MemoryAccess(AccessPhi, BB), DefVersion(DV) {}<br>
----------------<br>
Given we know the number of predecessors, we should reserve Args space.<br></blockquote><div><br></div><div>I"m not sure what you mean here, could you expand? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:239<br>
@@ +238,3 @@<br>
+  // the nearest dominating clobbering Memory Access (by skipping non-aliasing<br>
+  // def links)<br>
+  MemoryAccess *getClobberingMemoryAccess(Instruction *);<br>
----------------<br>
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.</blockquote><div>Yes, it can.</div><div> I'm happy to do whatever people think is best, i don't have a strong opinion.</div><div>In GCC, it's a separate API.</div><div><br></div><div>I'll note that if we do the optimization sanjoy suggested (which seems beneficial for gvn), we do need this, at least privately to the class.</div><div><br></div><div>For the moment i've guarded that with OPTIMIZE_USES in the diff i'm about to upload, clearly i'll remove it once a consensus/decision is reached.</div><div><br></div></div></div></div></div>