This is an archive of the discontinued LLVM Phabricator instance.

[NFCI][LoopUnrollAndJam] Changing LoopUnrollAndJamPass to a function pass.
ClosedPublic

Authored by Whitney on Jan 5 2020, 9:23 AM.

Details

Summary

This patch changes LoopUnrollAndJamPass to a function pass, and keeps the loops traversal order same as defined in FunctionToLoopPassAdaptor LoopPassManager.h.

The next patch will change the loop traversal to outer to inner order, so more loops can be transform.

Discussion in llvm-dev mailing list: https://groups.google.com/forum/#!topic/llvm-dev/LF4rUjkVI2g

Diff Detail

Event Timeline

Whitney created this revision.Jan 5 2020, 9:23 AM

Thanks.

This is only the new pass manager, right? It's probably best to keep the old pass manager working in the same way too, to keep them in sync. That will be the one that we use downstream at the mo.

Whitney updated this revision to Diff 236432.Jan 6 2020, 12:05 PM

Changing LoopUnrollAndJamPass to a function pass for old pass manager as well.

dmgreen added inline comments.Jan 6 2020, 2:50 PM
llvm/lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp
440

What if the function has no loops?

Whitney updated this revision to Diff 236472.Jan 6 2020, 3:15 PM
Whitney marked an inline comment as done.

Addressed review comment.

If UnJ needs loops in LoopSimplify form then it should request (or enforce) that loops are in that form. I think it can be done with AU.addRequiredID(LoopSimplifyID); (and maybe a DEPENDENCY on LoopSimplify?) That should hopefully mean that the tests don't need these adjustments.

Do we have any tests for the new pass manager? I feel we would have added some, but I don't see any here.

llvm/lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp
441

I feel like we may need LCSSA on the outermost loops before calling UnJ on the inner one, from the loop of the check on line 576 of LoopUnrollAndJam.cpp. I'm not sure though, I didn't see it crash anywhere, but we likely don't have a good testcase for this.

If UnJ needs loops in LoopSimplify form then it should request (or enforce) that loops are in that form. I think it can be done with AU.addRequiredID(LoopSimplifyID); (and maybe a DEPENDENCY on LoopSimplify?) That should hopefully mean that the tests don't need these adjustments.

I am not familiar with addRequiredID, do you know if there is an equivalent in the new pass manager?

Do we have any tests for the new pass manager? I feel we would have added some, but I don't see any here.

I don't see any LIT tests using the new pass manager in llvm/test/Transforms/LoopUnrollAndJam, is it in some other folder I missed?

Whitney marked an inline comment as done.Jan 7 2020, 1:11 PM
Whitney added inline comments.
llvm/lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp
441

I am thinking that we can treat LCSSA the same as loop simplify, if the loop is not in LCSSA form, then give up. And we can run LoopSimplify pass and LCSSA pass before unroll and jam pass. What do you think?

If UnJ needs loops in LoopSimplify form then it should request (or enforce) that loops are in that form. I think it can be done with AU.addRequiredID(LoopSimplifyID); (and maybe a DEPENDENCY on LoopSimplify?) That should hopefully mean that the tests don't need these adjustments.

I am not familiar with addRequiredID, do you know if there is an equivalent in the new pass manager?

Do we have any tests for the new pass manager? I feel we would have added some, but I don't see any here.

I don't see any LIT tests using the new pass manager in llvm/test/Transforms/LoopUnrollAndJam, is it in some other folder I missed?

Not sure I'm afraid. I'm not much of an expert on the new pass manager (whenever I've tried it here, it's made performance worse and codesize bigger! So haven't used it much)

Please add the tests if it's simple enough. If it looks like it might be trouble, feel free to leave that to another patch.

llvm/lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp
441

Sounds good. I think (but may be mistaken) that LCSSA can't "fail" to be created in the same way as LoopSimplify can. It looks like we can request them and mark them as preserved in the same way, though.

If UnJ needs loops in LoopSimplify form then it should request (or enforce) that loops are in that form. I think it can be done with AU.addRequiredID(LoopSimplifyID); (and maybe a DEPENDENCY on LoopSimplify?) That should hopefully mean that the tests don't need these adjustments.

In https://groups.google.com/forum/m/?fbclid=IwAR3jQc7xQaWv2ovAvFTNuZXFKvCBo5lRx-vt_kyomdmcBgYpcRy5YcaDa0s#!topic/llvm-dev/0K8gEZ22VOE,
You cannot reliably use the addRequired() method to force a transform pass to run before another pass
The addRequired method is designed to tell the PassManager which *analysis* passes to run before your pass.

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?

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.

Whitney updated this revision to Diff 236851.Jan 8 2020, 9:54 AM
Whitney marked 2 inline comments as done.
  1. Makes loops in simplified and LCSSA form in the beginning of LoopUnrollAndJamPass.
  2. Add LITs for new pass manager.
dmgreen accepted this revision.Jan 9 2020, 1:28 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Jan 9 2020, 1:28 AM
This revision was automatically updated to reflect the committed changes.