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

[Daniel Berlin]

>>> ================
>>> Comment at: lib/Transforms/Utils/MemorySSA.cpp:327
>>> @@ +326,3 @@
>>> +      }
>>> +      if (def) {
>>> +        MemoryDef *MD = new MemoryDef(nullptr, &I, &B, nextID++);
>>> ----------------
>>> else if - once you do that, can't this just become:
>>> if (def) {
>>> } else if (use) {
>>> } else {
>>>    llvm_unreachable()
>>> }
>>>
>> It can be simplified to if (def) ... else if (use), but you can't add
>> an unreachable, because it could be NoModRef
>
> It's also worth commenting that NoModRef is possible wherever this code
> ended up.  I'd completely missed that when reading through.

Fixed.

>>
>>
>>> ================
>>> Comment at: lib/Transforms/Utils/MemorySSA.cpp:560
>>> @@ +559,3 @@
>>> +  // may now be defined in terms of it.
>>> +  bool usedByReplacee = getDefiningAccess(Replacer) == Replacee;
>>> +
>>> ----------------
>>> This condition really doesn't feel like a "replacement".  Is it time to
>>> reconsider the interface and turn this into an assert?
>>
>> Yeah, this happens in store merging, where the phi becomes the new
>> defining access for the use.
>>
>> I will add a "addNewAccessWithDefiningAccess" to handle this case,
>> instead of replace.
>>
>> It is actually a lot easier to express it as a series of low level
>> operations, but i was trying to make sure MemorySSA is valid at each
>> step of updating.
>>
>> Honestly, the updater API's are much more in flux than any of the
>> other ones, because they essentially have two users right now, and
>> while i tried to generalize them as much as i thought would be
>> necessary, i could be completely wrong :)
>>
>> They are also harder to test.
>>
>> Also, today I realized we will need a cache invalidation API pretty
>> quickly for the walkers, which is going to require a bit of
>> thinking/iteration.
>> These API should not be required for basic use, only for preserving
>> MemorySSA.
>>
>> As such, I think I will remove them from the first committed version,
>> and then submit them again for review with unit tests before the new
>> MergedLoadStoreMotion goes in.
>
> SGTM


>>
>>
>>> ================
>>> Comment at: lib/Transforms/Utils/MemorySSA.cpp:821
>>> @@ +820,3 @@
>>> +
>>> +void MemorySSAPrinterPass::registerOptions() {
>>> +  OptionRegistry::registerOption<bool, MemorySSAPrinterPass,
>>> ----------------
>>> Can't you just use cl opt for this?
>>
>> Yes, my understanding was "this was the new way to do this because it
>> makes the command line options scoped to the pass" instead of global.
>
> Ah, it might be.  I have no clue.  I haven't seen any announcements or doc
> changes.
>>>
>>> ================
>>> Comment at: lib/Transforms/Utils/MemorySSA.cpp:942
>>> @@ +941,3 @@
>>> +  // Don't try to walk past an incomplete phi node during construction
>>> +  if (P->getNumIncomingValues() != P->getNumPreds())
>>> +    return std::make_pair(P, false);
>>> ----------------
>>> Er, huh?  How can this happen?  And should it?
>>
>> So, we use the caching walker during construction to optimize the uses
>> on MemoryUse's.
>> Depending on where the backedges are, you have no guarantee you won't
>> hit a PHI node whose operands are not completely filled in yet,
>> because the standard SSA rename algorithm is  top-down, and depth
>> first, not breadth first  (so along one branch, you check if your
>> successors have phi node, and if so, fill in your open operand.   Then
>> you go to your successors, who may want to make queries involving that
>> phi node, but not all operands are filled in because the renamer
>> hasn't gotten to the *other* branch yet)
>>
>> Even if we made it breadth first, my gut tells me you can construct a
>> CFG with backedges that will end up with not all operands filled in at
>> some query point.
>
> A clear comment in function description which says it has to handle
> partially constructed memssa would be very helpful.  I understand why you
> want this, but it really makes me wince.
>
> Any chance the optimization could be done via a lazy fixup after
> construction?  Just a though.  (Not as part of this change though!)
>

So it can be done by fixup, at a cost of walking the memory accesses again.
The only way to do it with *lazy* fixup would be to make the walkers
modify the IR as they are walking it.
Currently they are readonly.
Honestly, that idea scares the hell out of me, so i'd rather either
accept that we may walk partially constructed phi nodes during
construction, or do at as a memory-access walk after construction.