[PATCH] D97290: [LoopInterchange] Replace tightly-nesting-ness check with the one from `LoopNest`

[Ta-Wei Tu via Phabricator via llvm-commits]

TaWeiTu added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:453
   bool run(LoopNest &LN) {
-    const auto &LoopList = LN.getLoops();
-    for (unsigned I = 1; I < LoopList.size(); ++I)
-      if (LoopList[I]->getParentLoop() != LoopList[I - 1])
-        return false;
-    return processLoopList(LoopList);
+    if (!LN.isTotallyNested())
+      return false;
----------------
Whitney wrote:
> TaWeiTu wrote:
> > Whitney wrote:
> > > TaWeiTu wrote:
> > > > Whitney wrote:
> > > > > As `checkLoopsStructure` is going to check `the inner loop should be the outer loop's only child`, do we really need to check `isTotallyNested` here?
> > > > > 
> > > > I think the logic would be different if we remove `isTotallyNested`. For example, consider the following loop-nest:
> > > > 
> > > > ```
> > > > for (i)
> > > >   for (j1)
> > > >     for (k)
> > > >       for (l)
> > > >   for (j2)
> > > > ```
> > > > 
> > > > Before removing `isTotallyNested`, `k` and `l` can never be interchanged. But since we only check whether the two loops that are currently being interchanged are tightly-nested or not, `k` and `l` might get interchanged before realizing that `i` has two subloops, in which case we simply return from `processLoopList` without unrolling the changes.
> > > > 
> > > > I'm not sure whether `LoopInterchange` is intended to operate **only** on "totally nested" loops or not, because in the previous example swapping `k` and `l` does seem feasible.
> > > > Anyhow, if such improvement is what we want, I think it will be better to have a separate patch for that.
> > > > 
> > > > What do you think? 
> > > > Thanks!
> > > If we only want to operate on perfect loop nest, then we can check `LN.getMaxPerfectDepth() == LN.getNestDepth()` here.
> > > Then we don't need to add function `isTotallyNested`.
> > Yes, but currently the definition of tightly nested loops in `LoopInterchange` is weaker than the perfectly nested loops defined in `LoopNest`.
> > Replacing `isTotallyNested` with `LN.getMaxPerfectDepth() == LN.getNestDepth()` loses quite a lot of optimization chances IIRC.
> I think we should try to loosen the the definition of perfect nest in loop nest first, instead of introducing a new term. 
> How about we keep the code:
> ```
>     const auto &LoopList = LN.getLoops();
>     for (unsigned I = 1; I < LoopList.size(); ++I)
>       if (LoopList[I]->getParentLoop() != LoopList[I - 1])
>         return false;
> ```
> and not introduce `isTotallyNested` in this patch?
> Other changes in this patch LGTM.
Sure, removed `isTotallyNested`.
Will try making the two definitions compatible as well.
Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97290