This is an archive of the discontinued LLVM Phabricator instance.

[PM] Split LoopUnrollPass and make partial unroller a function pass
ClosedPublic

Authored by tejohnson on Aug 1 2017, 9:13 AM.

Details

Summary

This is largely NFC*, in preparation for utilizing ProfileSummaryInfo
and BranchFrequencyInfo analyses. In this patch I am only doing the
splitting for the New PM, but I can do the same for the legacy PM as
a follow-on if this looks good.

*Not NFC since for partial unrolling we lose the updates done to the
loop traversal (adding new sibling and child loops) - according to
Chandler this is not very useful for partial unrolling, but it also
means that the debugging flag -unroll-revisit-child-loops no longer
works for partial unrolling.

A couple other notes/questions:

  1. I noticed that we are still doing partial unrolling via the new

full unroll pass when there is metadata directing a partial unroll
(see @partial_unroll in test/Transforms/LoopUnroll/revisit.ll).
Should I change the code so this is disallowed when unrolling occurs via
the new full unroll pass? If so, presumably this part of the test
becomes obsolete.

  1. The LoopPassManager adds a couple of loop canonicalization passes

(LoopSimplifyPass and LCSSAPass). I found I had to add these to
the list of passes for the test/Transforms/LoopUnroll/runtime-loop*.ll
tests to get the same behavior from the partial unroller since it no
longer had those added automatically. In the pipelines we set up in the
PassBuilder, should these be added before the function pass
LoopUnrollPass is added, or can we assume that we would have already
have done any necessary canonicalization through some earlier
invocations of the LoopPassManager?

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson created this revision.Aug 1 2017, 9:13 AM
chandlerc added inline comments.Aug 1 2017, 2:09 PM
include/llvm/Transforms/Scalar/LoopUnrollPass.h
24 ↗(On Diff #109138)

This comment seems a bit stale now.

31 ↗(On Diff #109138)

I think it would be good to talk about the fact that this is a function pass rather than a loop pass in order to have access to analyses and control over the loop nest.

Also (see below for details) probably should mention that it will additionally put loops into canonical (simplified + LCSSA) form

lib/Transforms/Scalar/LoopUnrollPass.cpp
1217–1218 ↗(On Diff #109138)

Since you always call this with an empty vector, maybe just return it by value?

1247 ↗(On Diff #109138)

I would directly simplify the loops and form LCSSA here so that you have the requirements for unrolling. That's similar to what the vectorizer does, although it doesn't seem to care about LCSSA and the unroller will.

1249–1253 ↗(On Diff #109138)

Mention somewhere the ordering?

tejohnson marked 5 inline comments as done.Aug 2 2017, 9:57 AM
tejohnson added inline comments.
include/llvm/Transforms/Scalar/LoopUnrollPass.h
24 ↗(On Diff #109138)

Removed. I initially left it in since it is still invoking the same underlying unrolling facility, but disabling runtime and partial unrolling.

As an aside, I just discovered that peeling is *not* disabled, and so the complete unroller is performing profile based peeling. I will send a follow on patch to disable that as well, since I want this one to be as NFC as possible.

lib/Transforms/Scalar/LoopUnrollPass.cpp
1247 ↗(On Diff #109138)

Done, and removed the places where I explicitly added loop-simplify,lcssa to the pass manger invocations in the runtime-loop*.ll tests.

tejohnson updated this revision to Diff 109368.Aug 2 2017, 9:59 AM
tejohnson marked 2 inline comments as done.

Address review comments

tejohnson updated this revision to Diff 109371.Aug 2 2017, 10:13 AM

Fix NDEBUG build - guard declaration of value used only in DEBUG build.

chandlerc accepted this revision.Aug 2 2017, 11:56 AM

Looks great, thanks for this restructuring.

This revision is now accepted and ready to land.Aug 2 2017, 11:56 AM
This revision was automatically updated to reflect the committed changes.