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

[Daniel Berlin]

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>
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


> ================
> 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.





> ================
> 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?

>
> ================
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150225/9db6b046/attachment.html>