[PATCH] D22966: GVN-hoist: compute MSSA once per function (PR28670)
Daniel Berlin via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 29 20:51:23 PDT 2016
Okay. I'll clean it up and add to the unittests and submit it.
On Fri, Jul 29, 2016 at 8:46 PM, Sebastian Pop <sebpop at gmail.com> wrote:
> 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);
> >> >>>>
> >> >>>>
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160729/d82f2c2b/attachment.html>
More information about the llvm-commits
mailing list