[PATCH] D76132: [LoopUnrollAndJam] Changed safety checks to consider more than 2-levels loop nest.

[Whitney Tsang via Phabricator via llvm-commits]

Whitney marked 4 inline comments as done.
Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:760-761
+      SmallVector<Value *, 4> DstLoadsAndStores;
+      if (!getLoadsAndStores(*I, SrcLoadsAndStores) ||
+          !getLoadsAndStores(*J, DstLoadsAndStores))
+        return false;
----------------
dmgreen wrote:
> Whitney wrote:
> > Whitney wrote:
> > > dmgreen wrote:
> > > > This looks like it would recalculate LoadsAndStores a lot? Src and Dst don't seem like the right nomenclature too, but that's just a small nit.
> > > Renamed, and removed the calculation of one of the LoadsAndStores in the outer loop. I considered creating a map, but the code may get complicated. Is the current change okay with you, or you prefer a map?
> > Renamed, and moved the calculation of one of the LoadsAndStores in the outer loop. I considered creating a map, but the code may get complicated. Is the current change okay with you, or you prefer a map?
> You may be right. It looks O(n^2), but perhaps n will not be too high?
n is `(loopdepth - 1) * 2 + 1`. I personally think is ok.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:914
   // Make sure we can move all instructions we need to before the subloop
+  BasicBlock *Header = L->getHeader();
+  BasicBlock *Latch = L->getLoopLatch();
----------------
dmgreen wrote:
> Whitney wrote:
> > dmgreen wrote:
> > > Should this have a check for a 2 deep loop nest at the moment (like before), if the remainder of the analysis/transform code hasn't been updated yet? It looks like the count calculation might just exclude anything with multiple subloop blocks at the moment anyway, so is possibly not a problem in practice, without pragma's.
> > Not sure I understand, this check is already like before. 
> Sorry, the actual line I was pointing to was semi-random. That wasn't clear.
> 
> Does the processHeaderPhiOperands check below need to check each level? IIRC it's testing data-dependencies (as in ssa/use-def dependencies, as opposed to the memory dependencies in checkDependencies. Can we physically move any instruction we need from aft to fore). If we end up moving multiple levels past one another, do we have to make the same checks at each level?
> 
> My general point was that some of the code still only handles 2-deep loop nests. Should we have a check somewhere (perhaps with a fixme next it) that still tests for that condition, until the rest of the code has caught up?
```
    Because of the way we rearrange basic blocks, we also require that
    the Fore blocks of L on all unrolled iterations are safe to move before the
    blocks of the direct child of L of all iterations. So we require that the
    phi node looping operands of ForeHeader can be moved to at least the end of
    ForeEnd, so that we can arrange cloned Fore Blocks before the subloop and
    match up Phi's correctly.
```
As we are only unrolling L, not its child, we don't need to move instructions from non-L AftBlock to non-L ForeBlock, so we don't need to check if the moves are safe. 

> My general point was that some of the code still only handles 2-deep loop nests. Should we have a check somewhere (perhaps with a fixme next it) that still tests for that condition, until the rest of the code has caught up?
I modified all checks I found needed in `isSafeToUnrollAndJam`, am I missing something?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76132/new/

https://reviews.llvm.org/D76132