[PATCH] D72230: [NFCI][LoopUnrollAndJam] Changing LoopUnrollAndJamPass to a function pass.

Dave Green via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 8 06:53:41 PST 2020


dmgreen added a comment.

In D72230#1809519 <https://reviews.llvm.org/D72230#1809519>, @Whitney wrote:

> The discussion in the link above sounds like using `addRequiredID` on a transformation pass is not a good idea. Beside that, I don't know a way to require transformations to run before a pass in the new pass manager.
>  I suggested to give up on loops that are not simplified or loop nests that are not in LCSSA.  And change the LITs to be all in simplified and LCSSA form. In additional, add the two transformation passes before LoopUnrollAndJamPass in the pipeline. What do you think?


I would agree that depending on other transform passes is a little odd, but it seems to work elsewhere. How do other passes work? LoopLoadElimination seems to be a FunctionPass that requires loops be in LoopSimplifyForm. I see it using `INITIALIZE_PASS_DEPENDENCY(LoopSimplify)` and `AU.addRequiredID(LoopSimplifyID);`

The LoopUnrollPass in the new pass manager seems to be a function pass now, and has this (same for the vectorizer):

  // The unroller requires loops to be in simplified form, and also needs LCSSA.
  // Since simplification may add new inner loops, it has to run before the
  // legality and profitability checks. This means running the loop unroller
  // will simplify all loops, regardless of whether anything end up being
  // unrolled.
  for (auto &L : LI) {
    Changed |=
        simplifyLoop(L, &DT, &LI, &SE, &AC, nullptr, false /* PreserveLCSSA */);
    Changed |= formLCSSARecursively(*L, DT, &LI, &SE);
  }

Can we steal that? I would prefer it if this was mostly automatic, not having to update the tests or be careful to run passes in the right order every time.

>From poking around, you may also need `-aa-pipeline=type-based-aa,basic-aa` to some of the tests if you run them under the new pass manager. And the order of addRequiredID seems to matter to get it to work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72230





More information about the llvm-commits mailing list