[PATCH] D22966: GVN-hoist: compute MSSA once per function (PR28670)

[Sebastian Pop via llvm-commits]

I think I have a patch that works around the limitation of only one
MSSA per instruction:
instead of moving the instruction that we hoist, we clone it, such
that it can have its own separate MSSA annotation.


On Fri, Jul 29, 2016 at 10:23 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
> Okay, so ...
>
> you have to create before you can remove.
>
> remove is going to destroy the memory, and not much we can do about that.
> (We could split remove into remove and erase, but ... ugly)
>
> If you create first, right now, it will do the wrong thing (the annotation
> will never end up on the instruction, unless the instruction is new)
> Please try the attached, which should make it stop doing the wrong thing
> there. If that works, i'll add unit tests and commit it.
>
> verifyMemorySSA will catch if you mess it up (verifyOrdering checks that the
> accesslist and the lookups have the same number of things).
>
>
> On Fri, Jul 29, 2016 at 7:36 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>> So, I think remove from lookups should check and only remove from the
>> lookup if the lookups point to that access
>>
>>
>> On Fri, Jul 29, 2016, 7:23 PM Daniel Berlin <dberlin at dberlin.org> wrote:
>>>
>>> I have to stare at this.
>>>
>>> On Fri, Jul 29, 2016, 7:10 PM Sebastian Pop <sebpop at gmail.com> wrote:
>>>>
>>>> sebpop updated this revision to Diff 66212.
>>>> sebpop added a comment.
>>>>
>>>> Updated the patch as I first wrote it: I think this is the natural way
>>>> to update an SSA variable.
>>>>
>>>> This fails because when invoking:
>>>> +          MSSA->removeMemoryAccess(OldMemAcc);
>>>>
>>>> we go from:
>>>> ; MemoryUse(2)
>>>>
>>>>   %2 = load i32, i32* %v.addr, align 4
>>>>
>>>> to no annotation at all:
>>>>
>>>>   %2 = load i32, i32* %v.addr, align 4
>>>>
>>>> This is because removeMemoryAccess calls removeFromLookups which does:
>>>>
>>>>   Value *MemoryInst;
>>>>   if (MemoryUseOrDef *MUD = dyn_cast<MemoryUseOrDef>(MA)) {
>>>>     MemoryInst = MUD->getMemoryInst();
>>>>   } else {
>>>>     MemoryInst = MA->getBlock();
>>>>   }
>>>>   ValueToMemoryAccess.erase(MemoryInst);
>>>>
>>>> and erase() removes both the new and old MemoryAccess'es leaving the
>>>> instruction without MSSA annotation.
>>>
>>>
>>> Yes, only one memory access can exist for an instruction for real.  You
>>> can't create two at once and get the right result.
>>>
>>>>
>>>> For loads this can be fixed by first removing the OldMemoryAccess and
>>>> then creating the NewMemoryAccess.
>>>> This works for loads because there are no users of a MemoryUse.
>>>> However for a store, we need to keep both the old and new
>>>> MemoryAccess'es at the same time, to be able to rename the uses of old with
>>>> new.
>>>
>>>
>>> Yes, most of the hoisters are cloning, so do not have this issue.
>>>
>>>>
>>>> I think the solution would be for "MSSA->removeMemoryAccess(OldMemAcc);"
>>>> to only invalidate and remove "OldMemAcc" without touching "NewMemAcc" that
>>>> should remain attached to the instruction.
>>>
>>>
>>> We can't make it work with two memoryacceses for the same instruction at
>>> once.
>>>
>>> We can probably implement either a createReplacement (that takes over the
>>> uses of the old one) or a moveBefore/moveAfter.
>>>>
>>>>
>>>>
>>>> https://reviews.llvm.org/D22966
>>>>
>>>> Files:
>>>>   lib/Transforms/Scalar/GVNHoist.cpp
>>>>
>>>> Index: lib/Transforms/Scalar/GVNHoist.cpp
>>>> ===================================================================
>>>> --- lib/Transforms/Scalar/GVNHoist.cpp
>>>> +++ lib/Transforms/Scalar/GVNHoist.cpp
>>>> @@ -193,30 +193,27 @@
>>>>      VN.setAliasAnalysis(AA);
>>>>      VN.setMemDep(MD);
>>>>      bool Res = false;
>>>> +    MemorySSA M(F, AA, DT);
>>>> +    MSSA = &M;
>>>>
>>>>      // FIXME: use lazy evaluation of VN to avoid the fix-point
>>>> computation.
>>>>      while (1) {
>>>> -      // FIXME: only compute MemorySSA once. We need to update the
>>>> analysis in
>>>> -      // the same time as transforming the code.
>>>> -      MemorySSA M(F, AA, DT);
>>>> -      MSSA = &M;
>>>> -
>>>>        // Perform DFS Numbering of instructions.
>>>>        unsigned I = 0;
>>>>        for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))
>>>>          for (auto &Inst: *BB)
>>>>            DFSNumber.insert({&Inst, ++I});
>>>>
>>>>        auto HoistStat = hoistExpressions(F);
>>>> -      if (HoistStat.first + HoistStat.second == 0) {
>>>> +      if (HoistStat.first + HoistStat.second == 0)
>>>>          return Res;
>>>> -      }
>>>> -      if (HoistStat.second > 0) {
>>>> +
>>>> +      if (HoistStat.second > 0)
>>>>          // To address a limitation of the current GVN, we need to rerun
>>>> the
>>>> -        // hoisting after we hoisted loads in order to be able to hoist
>>>> all
>>>> -        // scalars dependent on the hoisted loads. Same for stores.
>>>> +        // hoisting after we hoisted loads or stores in order to be
>>>> able to
>>>> +        // hoist all scalars dependent on the hoisted ld/st.
>>>>          VN.clear();
>>>> -      }
>>>> +
>>>>        Res = true;
>>>>
>>>>        // DFS numbers change when instructions are hoisted: clear and
>>>> recompute.
>>>> @@ -725,6 +722,8 @@
>>>>            if (!Repl || firstInBB(I, Repl))
>>>>              Repl = I;
>>>>
>>>> +      MemoryAccess *NewMemAcc = nullptr;
>>>> +
>>>>        if (Repl) {
>>>>          // Repl is already in HoistPt: it remains in place.
>>>>          assert(allOperandsAvailable(Repl, HoistPt) &&
>>>> @@ -742,7 +741,23 @@
>>>>              !makeGepOperandsAvailable(Repl, HoistPt,
>>>> InstructionsToHoist))
>>>>            continue;
>>>>
>>>> +        // Before moving the instruction, get its MSSA access.
>>>> +        MemoryUseOrDef *OldMemAcc = nullptr;
>>>> +        if (MemoryAccess *MA = MSSA->getMemoryAccess(Repl))
>>>> +          OldMemAcc = dyn_cast<MemoryUseOrDef>(MA);
>>>> +
>>>> +        // Move the instruction at the end of HoistPt.
>>>>          Repl->moveBefore(HoistPt->getTerminator());
>>>> +
>>>> +        if (OldMemAcc) {
>>>> +          // The definition of this ld/st will not change: ld/st
>>>> hoisting is
>>>> +          // legal when the ld/st is not moved past its current
>>>> definition.
>>>> +          MemoryAccess *Def = OldMemAcc->getDefiningAccess();
>>>> +          NewMemAcc = MSSA->createMemoryAccessInBB(Repl, Def, HoistPt,
>>>> +                                                   MemorySSA::End);
>>>> +          OldMemAcc->replaceAllUsesWith(NewMemAcc);
>>>> +          MSSA->removeMemoryAccess(OldMemAcc);
>>>> +        }
>>>>        }
>>>>
>>>>        if (isa<LoadInst>(Repl))
>>>> @@ -775,6 +790,14 @@
>>>>            } else if (isa<CallInst>(Repl)) {
>>>>              ++NumCallsRemoved;
>>>>            }
>>>> +
>>>> +          if (NewMemAcc) {
>>>> +            // Update the uses of the old MSSA access with NewMemAcc.
>>>> +            MemoryAccess *OldMA = MSSA->getMemoryAccess(I);
>>>> +            OldMA->replaceAllUsesWith(NewMemAcc);
>>>> +            MSSA->removeMemoryAccess(OldMA);
>>>> +          }
>>>> +
>>>>            Repl->intersectOptionalDataWith(I);
>>>>            combineKnownMetadata(Repl, I);
>>>>            I->replaceAllUsesWith(Repl);
>>>>
>>>>
>