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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

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

> In D72230#1808620 <https://reviews.llvm.org/D72230#1808620>, @dmgreen wrote:
>
> > 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.



================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:441
+    Loop *L = Worklist.pop_back_val();
+    formLCSSA(*L, DT, &LI, &SE);
+    LoopUnrollResult Result =
----------------
Whitney wrote:
> dmgreen wrote:
> > 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.
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72230