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

[Xinliang David Li via llvm-commits]

On Sat, Jan 30, 2016 at 8:36 AM, Daniel Berlin <dberlin at dberlin.org> wrote:
>
>
> 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 :)
>

I see. Efficiency is certainly more important than purity. The reason
I proposed that change is George's memory overhead concern -- the
Users list currently has to be stored in the base class. The fixed
cost per MemoryAccess using SmallPtrSet is 64 bytes.


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

yes -- it sounds expensive :)

(It may not actually end up being too bad though -- looks like many
caller contexts of this method already do some dispatching using
dyn_cast (e.g def iterator's operator* method).

thanks,

David


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