[PATCH] D23032: Rewrite the use optimizer to be less memory intensive and 50% faster.Fixes PR28670

[Daniel Berlin via llvm-commits]

On Tue, Aug 2, 2016 at 12:53 PM, Sebastian Pop <sebpop at gmail.com> wrote:

> sebpop added a subscriber: sebpop.
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1116
> @@ +1115,3 @@
> +      return CS.getCalledValue() == Other.CS.getCalledValue();
> +    else
> +      return Loc == Other.Loc;
> ----------------
> No need for "else" after return.
>
> Fixed.


> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1139
> @@ +1138,3 @@
> +                              MLOC.getCS().getCalledValue()));
> +    else
> +      return hash_combine(
> ----------------
> Remove "else".
>
> Fixed


> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1246
> @@ +1245,3 @@
> +    auto &LocInfo = LocStackInfo[UseMLOC];
> +    // If the pop epoch changed, if means we've removed stuff from top of
> +    // stack due to changing blocks. We may have to reset the lower bound
> or
> ----------------
> s/if means/it means/
>
> Fixed.


> ================
> Comment at: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp:1152
> @@ +1151,3 @@
> +/// is a batch walker that touches everything, it does not operate like
> the
> +/// other walkers.  This walker is basically performing a top-down SSA
> renaming
> +/// pass, where the version stack is used as the cache.  This enables it
> to be
> ----------------
> Would it be possible to merge the UseOptimizer with the renamePass?
>


No, because phi nodes are not guaranteed complete until after renaming is
done.

(more particularly, there is no ordering in which to do renaming that can
guarantee you don't hit incomplete phi nodes).

At one point, we had use optimization be part of renaming, but walking
incomplete phi nodes is ....
complicated and not guaranteed to get right answers.



> ================
> Comment at: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp:1284
> @@ +1283,3 @@
> +      DEBUG(dbgs() << "We are being asked to check up to "
> +                   << UpperBound - LocInfo.LowerBound
> +                   << " loads and stores, so we didn't.\n");
> ----------------
> This message needs some rewording.
>
Fixed.


>
> ================
> Comment at: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp:1462
> @@ -1163,21 +1461,3 @@
>    Walker->setAutoResetWalker(false);
> -
> -  // Now optimize the MemoryUse's defining access to point to the nearest
> -  // dominating clobbering def.
> -  // This ensures that MemoryUse's that are killed by the same store are
> -  // immediate users of that store, one of the invariants we guarantee.
> -  for (auto DomNode : depth_first(DT)) {
> -    BasicBlock *BB = DomNode->getBlock();
> -    auto AI = PerBlockAccesses.find(BB);
> -    if (AI == PerBlockAccesses.end())
> -      continue;
> -    AccessList *Accesses = AI->second.get();
> -    for (auto &MA : *Accesses) {
> -      if (auto *MU = dyn_cast<MemoryUse>(&MA)) {
> -        Instruction *Inst = MU->getMemoryInst();
> -        MU->setDefiningAccess(Walker->getClobberingMemoryAccess(Inst));
> -      }
> -    }
> -  }
> -
> +  OptimizeUses(this, Walker, AA, DT).optimizeUses();
>    Walker->setAutoResetWalker(true);
> ----------------
> If the renamePass (called just above) gets the right answer, there is no
> need to OptimizeUses ;-)
> Maybe the logic from OptimizeUses can be merged in the renamePass.
>
> You can look back at the history of the review of the original patches to
see a version where it is merged.
It turns out to be a really bad idea ;)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160802/37bf1571/attachment.html>