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

Philip Reames listmail at philipreames.com
Wed Feb 25 11:14:34 PST 2015


On 02/25/2015 10:55 AM, Daniel Berlin wrote:
> Thanks so much for taking a look at what i've got so far!
>
>
> On Wed, Feb 25, 2015 at 10:01 AM, Philip Reames 
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
>     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.
>
>
> Okay. Some of it is solid, some of it less so, so other people 
> thinking about it and looking at it are definitely wanted :)
>
> Let me just respond to a few questions (i'll clean up and integrate 
> your other suggestions directly today)================
>
>     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?
>
> Yes
>
> Suggestions for better name welcome
Possible getMemoryDef?  (With a clear comment that this is def in the 
sense of def-use, not def as in memory store)

Or: getMemSSADef to be slightly more verbose?

>
>     ================
>     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.)
>
>
> So, you are right that this is entirely convenience.
>
> 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.
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.)
>
>
>
>
>
>     ================
>     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.
>
>
> I"m not sure what you mean here, could you expand?
You have a push mechanism.  The number of predecessors of BB is fixed at 
construction time.  Something like:
Args.reserve(std::distance(pred_begin 
<http://llvm.org/docs/doxygen/html/namespacellvm.html#a7e108932dc3da5294aed99a353aac9c4>(BB),pred_end 
<http://llvm.org/docs/doxygen/html/namespacellvm.html#a5eeaf08e96168c2cac8960f87f0ef360>(BB)));

would help to avoid internal allocation in Args when pushing items on.
>
>
>     ================
>     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.
>
> Yes, it can.
>  I'm happy to do whatever people think is best, i don't have a strong 
> opinion.
> In GCC, it's a separate API.
>
> 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.
>
> 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.
Let's see how this evolves.  It's too early to tell.

Philip

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150225/be9c692d/attachment.html>


More information about the llvm-commits mailing list