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

[Daniel Berlin via llvm-commits]

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/20160730/504ddcf9/attachment.html>