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

[Sebastian Pop via llvm-commits]

Yes, your patch works without my last changes: i.e., keeping the
natural way of search/replace an SSA name.

On Fri, Jul 29, 2016 at 10:35 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
> FWIW: The patch i sent should fix the issue of being able to create them.
> The code right now is clearly wrong, it assumes but does not check that
> insertion never fails, etc.
>
>
> On Fri, Jul 29, 2016 at 8:30 PM, Sebastian Pop <sebpop at gmail.com> wrote:
>>
>> 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);
>> >>>>
>> >>>>
>> >
>
>