[PATCH] D16743: This patch hijacks Danny's MemorySSA, a virtual SSA form for memory. Details on what it looks like are in MemorySSA.h

[Daniel Berlin via llvm-commits]

On Fri, Jan 29, 2016 at 10:06 PM, David Li <davidxl at google.com> wrote:

> davidxl added inline comments.
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:16
> @@ +15,3 @@
> +// instructions such loads, stores, atomics and calls. Additionally, it
> does a
> +// trivial form of "heap versioning" Every time the memory state changes
> in the
> +// program, we generate a new heap version. It generates
> MemoryDef/Uses/Phis
> ----------------
> nit: missing period.
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:18
> @@ +17,3 @@
> +// program, we generate a new heap version. It generates
> MemoryDef/Uses/Phis
> +// that are overlayed on top of the existing instructions
> +//
> ----------------
> nit: missing period at the end.
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:186
> @@ +185,3 @@
> +
> +  // FIXME(gbiv): MemoryUses don't have users. The quick fix for this
> would be
> +  // to make a virtual `getUsers()` with llvm_unreachable in MemoryUse,
> but I'd
> ----------------
> In the context when user_begin() is called, the memoryAccess must be a
> def. Given that assumption, you can safely DownCast the memoryAccess to
> MemoryDef -- the problem is both Def and Phi need to be handled.
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:203
> @@ +202,3 @@
> +  //
> +  // ...But then virtual base classes happen (diamond between
> MemoryAccess and
> +  // Def), and those are icky. Not sure if the trait system would need
> virtual
> ----------------
> I have another suggestion of the hierarchy:
>
> Strictly speaking a MemoryDef  and MemoryUse is not the IsA relationship,
> but a hasA relationship -- basically a memory def is not a use, but it has
> any implicit may use to model the may-kill.


Correct.


>  With that in mind,  it is probably not a good idea to factor out UseOrDef
> out as the common ancestor of MemoryDef and MemoryUse. Instead:


> 1) MemoryDef should have a pointer to a memoryUse instance
>

This is expensive to indirect.  Seriously. I measured the slowdown at about
15-20% on common memorySSA algorithms.

MemorySSA dominates GVN time in NewGVN, and i expect, in most other
algorithms currently using memory dependence.
While it should not be optimized early, the current hierarchy is a
*deliberate* tradeoff between speed and theoretical OO modeling purity :)



> 2) MemoryDef and Phi Node should have a common ancestor DefOrPhi as drawn
> above.
>
> Pros: the userList can now be moved into DefOrPhi
> Cons: get|setDefininingAccess method now needs to be declared as virtual
> in MemoryAccess base and overridden in MemoryUse and MemoryDef.  For
> MemoryDef, it just need to forward the call to the memoryUse it owns.
>
> To avoid virtual call, the memoryAccess can do dispatching via DownCasting
> depending on memoryAccess type.
>
>
This seems expensive :)


> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:333
> @@ +332,3 @@
> +  const unsigned ID;
> +  UserListType Users;
> +};
> ----------------
> Is this redundant? There is one in base class.
>
>
It is redundant ;)

I put user lists in the base class deliberately.

Otherwise, due to the class hierarchy, you have to check for a phi or a def.

Whee.

(The next question that usually gets asked is why none of these classes are
derived from User/Value or something to handle the use lists
I spent a couple days trying to make this work but could not get it to work
right.  Better attempts completely welcome.
Part of the issue you hit is that none of these things are *really* part of
the IR, so a bunch of functions become quite unhappy.
It also wastes  8 bytes on the Type pointer that we don't have, need, or
use).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160130/fcf2f453/attachment.html>