[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:35:20 PDT 2016
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/cb90cf7b/attachment.html>
More information about the llvm-commits
mailing list