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

[Daniel Berlin via llvm-commits]

On Mon, Aug 1, 2016 at 2:47 PM, George Burgess IV <
george.burgess.iv at gmail.com> wrote:

> george.burgess.iv added a comment.
>
> Thanks for the patch!
>
> Did you get to run this across llvm/etc., or would you like me to do so?
>

I've run it across a large group of large and small testcases. I have found
no regressions.
It uses ~O(Number of instructions) memory on every testcase  i can find.
It averages about 30% faster (without the limiting).

It basically makes use optimization linear for non-phis.
With the limiting in place, it takes PR28760 from 30 seconds to 0.04
seconds to build memoryssa with no actual decrease in optimization.

The limiting is only there at all because of AA time (we make the optimal
number of AA queries already)

If you have a suite to run it on, happy to see it done.




>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:606
> @@ +605,3 @@
> +  // This is used by the use optimizer class
> +  AccessList *getWritableBlockAccesses(const BasicBlock *BB) const {
> +    auto It = PerBlockAccesses.find(BB);
> ----------------
> Can we change `getBlockAccesses` to just call this?
>


Yes.




>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1101
> @@ +1100,3 @@
> +
> +  bool isCall;
> +  ImmutableCallSite getCS() const {
> ----------------
> Nit: `IsCall`.
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1191
> @@ +1190,3 @@
> +
> +static bool instructionClobbersQuery(MemoryDef *MD, MemoryLocOrCall
> &UseMLOC,
> +                                     AliasAnalysis &AA) {
> ----------------
> Nit: Can we do `const MemoryLocOrCall &`?
>

we should be able to.


>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1226
> @@ +1225,3 @@
> +  // increment the PopEpoch to account for this.
> +  while (!DT->dominates(VersionStack.back()->getBlock(), BB)) {
> +    VersionStack.pop_back();
> ----------------
> Looks like we can query the DT a lot less if we delete all `VersionStack`
> elements with the same BB as `VersionStack.back()`. Unsure how expensive
> those queries are, though, so it may or may not matter.
>


I have no profile where DT shows up, because its' a constant time query,
but happy to do it if we care  :)



>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1231
> @@ +1230,3 @@
> +  for (MemoryAccess &MA : *Accesses) {
> +    if (auto *MU = dyn_cast<MemoryUse>(&MA)) {
> +      MemoryLocOrCall UseMLOC(MU);
> ----------------
> I think
> ```
> auto *MU = dyn_cast...;
> if (!MU) {
>   VersionStack.push_back(&MA);
>   ++StackEpoch;
>   continue;
> }
>
> //rest of the if block
> ```
>
> is more readable
>



>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1270
> @@ +1269,3 @@
> +
> +      if ((UpperBound - LocInfo.LowerBound) > MaxCheckLimit) {
> +        DEBUG(dbgs() << "We are being asked to check up to "
> ----------------
> Nit: Redundant parens
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1292
> @@ +1291,3 @@
> +          break;
> +        } else if (MemoryDef *MD =
> cast<MemoryDef>(VersionStack[UpperBound])) {
> +          if (instructionClobbersQuery(MD, UseMLOC, *AA)) {
> ----------------
> Please remove the `else`, since we always break from the above `if`.
>
> (Also, it looks like we can kill the `if`, since you're using `cast`)
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1319
> @@ +1318,3 @@
> +
> +/// Optimize uses to point to their actually clobbering definitions.
> +void MemorySSA::OptimizeUses::optimizeUses() {
> ----------------
> s/actually/actual/?
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1330
> @@ +1329,3 @@
> +  SmallVector<StackInfo, 16> DomTreeWorklist;
> +  DenseMap<MemoryLocOrCall, struct MemlocStackInfo> LocStackInfo;
> +  DomTreeWorklist.push_back({DT->getRootNode(),
> DT->getRootNode()->begin()});
> ----------------
> Nit: useless `struct`
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:2115
> @@ -1844,2 +2114,3 @@
>    MemoryAccess *Clobber = getClobberingMemoryAccess(DefiningAccess, Q);
> +#if 0
>    DEBUG(dbgs() << "Starting Memory SSA clobber for " << *I << " is ");
> ----------------
> Is this (and below) intentionally left in?
>

Ugh, no. I'll revert. We may or may not want to do this, as it makes for
very messy debug output when we have a large number of queries.



>
>
> https://reviews.llvm.org/D23032
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160801/49d9efec/attachment.html>