[PATCH] D51715: [LICM] Avoid duplicate work during building AliasSetTracker

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1545
   std::unique_ptr<AliasSetTracker> CurAST;
-  SmallVector<Loop *, 4> RecomputeLoops;
-  auto mergeLoop = [&CurAST](Loop *L) {
-    // Loop over the body of this loop, looking for calls, invokes, and stores.
-    for (BasicBlock *BB : L->blocks())
-      CurAST->add(*BB); // Incorporate the specified basic block
-  };
+  DenseSet<BasicBlock *> HandledBBs;
+
----------------
reames wrote:
> reames wrote:
> > I'm fine with this implementation, but if you wanted, you could structure this one of two ways:
> > 1) Use a SmallVector<BasicBlock> since the sets of BBs are distinct by assumption.
> > 2) Keep track of which loops are handle, then check the containing loop for each BB using LoopInfo (it has a map from BB to innermost containing loop).
> Or just inline mergeLoop and restore the check removed by cce6712519 for the parent loop.  That's the change which appears to have introduced this compile time regression.  :)
> 
> Yeah for compile time problems which go unfound for 20 months...
1. The main operation for HandledBBs we need is to find whether some BB is in this set. I do not think that SmallVector is a good candidate for that.
2. I do not think it is a right solution. When we go over all basic blocks of the loop L we can take a BB which is a part of inner inner loop (with depth = depth(L) + 2 or more. When we traversed inner loops we considered only immediate sub loops. So if we collect all used sub loops (AllSL) and now we consider BB then it is not enough to check that Loop(BB) is in ALLSL set. We must ensure that some element of ALLSL contains BB. This does not look like a less complexity than just collect all BBs and check whether it is already handled like I did in the patch.



https://reviews.llvm.org/D51715