<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 02/25/2015 10:55 AM, Daniel Berlin
wrote:<br>
</div>
<blockquote
cite="mid:CAF4BwTVeHcwmMv3BKhVRGrY4AYL7-+__b_aH_66BFxDk61D0Kg@mail.gmail.com"
type="cite">
<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
moz-do-not-send="true"
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>
</div>
</div>
</div>
</blockquote>
Possible getMemoryDef? (With a clear comment that this is def in
the sense of def-use, not def as in memory store)<br>
<br>
Or: getMemSSADef to be slightly more verbose?<br>
<br>
<blockquote
cite="mid:CAF4BwTVeHcwmMv3BKhVRGrY4AYL7-+__b_aH_66BFxDk61D0Kg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</div>
</div>
</blockquote>
I'm not opposed to this. :) However, I think you should document
the vector assumption. I'd have otherwise expected large unique
integers. Even if it's an implementation detail, it's an important
one for internal usage. From an external perspective, they should
be opaque ids. (I might even introduce a typedef to make that
clear.)<br>
<blockquote
cite="mid:CAF4BwTVeHcwmMv3BKhVRGrY4AYL7-+__b_aH_66BFxDk61D0Kg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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? <br>
</div>
</div>
</div>
</div>
</div>
</blockquote>
You have a push mechanism. The number of predecessors of BB is
fixed at construction time. Something like:<br>
Args.reserve(std::distance(<a class="code"
href="http://llvm.org/docs/doxygen/html/namespacellvm.html#a7e108932dc3da5294aed99a353aac9c4">pred_begin</a>(<span
class="keyword">BB</span>),<a class="code"
href="http://llvm.org/docs/doxygen/html/namespacellvm.html#a5eeaf08e96168c2cac8960f87f0ef360">pred_end</a>(<span
class="keyword">BB</span>)));<br>
<br>
would help to avoid internal allocation in Args when pushing items
on. <br>
<blockquote
cite="mid:CAF4BwTVeHcwmMv3BKhVRGrY4AYL7-+__b_aH_66BFxDk61D0Kg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</div>
</div>
</blockquote>
Let's see how this evolves. It's too early to tell.<br>
<br>
Philip<br>
<br>
</body>
</html>