Page MenuHomePhabricator

[LoopUnroll] allow customization for new-pass-manager version of LoopUnroll
ClosedPublic

Authored by fedor.sergeev on Oct 19 2018, 9:55 AM.

Details

Summary

Unlike its legacy counterpart new pass manager's LoopUnrollPass does
not provide any means to select which flavors of unroll to run
(runtime, peeling, partial), relying on global defaults.

In some cases having ability to run a restricted LoopUnroll that
does more than LoopFullUnroll is needed.

Introduced LoopUnrollOptions to select optional unroll behaviors.
Added 'unroll<peeling>' to PassRegistry mainly for the sake of testing.

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev created this revision.Oct 19 2018, 9:55 AM

I know that this change lacks testing, yet I have no idea on how to organize the test.
I'm doing this change for our downstream usage, so there currently no use of customized loop-unroll upstream and there are no command-line controls that could trigger it in a test.

Any suggestions on how to test it?

clang-formatted

I know that this change lacks testing, yet I have no idea on how to organize the test.
I'm doing this change for our downstream usage, so there currently no use of customized loop-unroll upstream and there are no command-line controls that could trigger it in a test.

Any suggestions on how to test it?

I guess you could use a unittest that creates the pass and invokes it. But probably easier would be to create multiple instances of the LoopUnrollPass in the PassRegistry.def, that construct the pass with different options (see my comment below about passing the options via the constructor), and then invoke the different types of unrolling via "opt -passes=" in a test. See for example in PassRegistry.def "early-cse" vs "early-cse-memssa".

include/llvm/Transforms/Scalar/LoopUnrollPass.h
49 ↗(On Diff #170214)

I think the usual mechanism for the new PM is to pass options to the pass constructor. E.g. see InlinerPass.

fedor.sergeev added inline comments.Oct 19 2018, 1:27 PM
include/llvm/Transforms/Scalar/LoopUnrollPass.h
49 ↗(On Diff #170214)

I really dislike passing a bunch of booleans and then adding comments to name them, like all the callers of tryToUnrollLoop are doing. :(

fedor.sergeev added inline comments.Oct 19 2018, 1:28 PM
include/llvm/Transforms/Scalar/LoopUnrollPass.h
49 ↗(On Diff #170214)

I can do something similar to SimplifyCFGOptions, passing the object that describes options to the constructor.

updated as per comments, tests added

fedor.sergeev edited the summary of this revision. (Show Details)Oct 23 2018, 6:01 AM
fedor.sergeev edited the summary of this revision. (Show Details)

one stray comment updated

chandlerc added inline comments.Oct 26 2018, 3:16 AM
include/llvm/Transforms/Scalar/LoopUnrollPass.h
42–45 ↗(On Diff #170617)

Any reason to allow this? Seems to just make it harder to correctly use...

49 ↗(On Diff #170617)

I'd fold this comment into a more general description of how to use this class in the class doxygen comment.

51 ↗(On Diff #170617)

Instead give a brief description of what behavior of the unroll pass it enables?

(Same for below methods...)

85 ↗(On Diff #170617)

Shouldn't the level move into the options struct as well?

lib/Passes/PassRegistry.def
220 ↗(On Diff #170617)

Rather than this, you can just make this pass support parsing parameters after the pass name? That would allow supporting more options.

Also will make it a bit cleaner as you can clearly start from a default state and adjust.

test/Transforms/LoopUnroll/runtime-loop.ll
19 ↗(On Diff #170617)

Make an initial update to factor in the common stuff here, using the existing flags? Then this patch will look a bit more focused.

fedor.sergeev added inline comments.Oct 27 2018, 12:02 PM
include/llvm/Transforms/Scalar/LoopUnrollPass.h
42–45 ↗(On Diff #170617)

Well, different people have different opinions.
Personally I would gladly drop this constructor :)

85 ↗(On Diff #170617)

Well... current solution makes this interface change "backward compatible", not breaking existing invocations.
Besides, OptLevel is rather different to all those UnrollOpts in that it does not describe a special transformation but is a rather nondescriptive tuning mechanism. To me that makes separating it quite logical.

lib/Passes/PassRegistry.def
220 ↗(On Diff #170617)

I'm not sure I follow.
Do you mean something like:

LoopUnrollPass(2).setPeeling(true) ?

updating as per comments: removing constructor, adding comments, splitting off NFC test update

fedor.sergeev marked 4 inline comments as done.Oct 29 2018, 7:47 AM
chandlerc added inline comments.Oct 30 2018, 1:42 AM
include/llvm/Transforms/Scalar/LoopUnrollPass.h
51 ↗(On Diff #171500)

I think more simple prose wolud be better here and below. For example:

"Enables or disables partial unrolling. When disabled, only full unrolling is allowed."

The "Selector for ... - ...." I think doesn't read easily.

85 ↗(On Diff #170617)

I'm not worried about API stability, especially here on a relatively new API. I'd prefer to get the API *right*.

I agree that the level based parameter isn't great, but I think it servers a purpose.

My suggestion would be to provide a factory function for the options struct which provides "reasonable defaults" of the options corresponding to optimization levels. It can make it clear that for full control, you should use the mutations, but that these give you starting positions which should be similar to what compilers that use the optimization levels provide.

lib/Passes/PassRegistry.def
220 ↗(On Diff #170617)

No, I'm suggesting teaching the pass pipeline parsing to actually parse parameters here:

unroll<O2>, unroll<partial,peeling,runtime>, unroll<O3,no-runtime>.

lib/Transforms/Scalar/LoopUnrollPass.cpp
1345–1346 ↗(On Diff #171500)

This last bit (or maybe the entire comment) seems a bit stale with the move to an options struct.

fedor.sergeev added inline comments.Oct 30 2018, 2:07 AM
include/llvm/Transforms/Scalar/LoopUnrollPass.h
51 ↗(On Diff #171500)

"Simple english prose" is not my strong point ;) Thanks for suggestion.

lib/Passes/PassRegistry.def
220 ↗(On Diff #170617)

Ah, okey, that should work.

chandlerc added inline comments.Oct 30 2018, 2:31 AM
include/llvm/Transforms/Scalar/LoopUnrollPass.h
51 ↗(On Diff #171500)

It's all good, I also struggle but in other directions of the same space. My English prose tends to ramble on and on (much like this comment), often saying far too little and completely failing to use commas correctly (also much like this comment). ;]

Honestly, *reviewing* for this is always much easier than writing it in the first place. So please don't ever feel bad for any comments in code review. I'm just really excited by all the work here.

updating as per comments, generic unroll parameters parsing part should be done as a followup soon after

fedor.sergeev edited the summary of this revision. (Show Details)Oct 30 2018, 8:13 AM
fedor.sergeev marked 3 inline comments as done.Oct 30 2018, 2:00 PM
chandlerc accepted this revision.Oct 31 2018, 3:00 AM

I think this is fine to go in, a minor suggestion inline. Rest can be follow-up.

lib/Passes/PassRegistry.def
218 ↗(On Diff #171705)

Any reason you can't just do the peeling bit? Seems overly verbose to reset the rest.

This revision is now accepted and ready to land.Oct 31 2018, 3:00 AM
fedor.sergeev added inline comments.Oct 31 2018, 3:15 AM
lib/Passes/PassRegistry.def
218 ↗(On Diff #171705)

Because it needs to be a restricted set, not allowing generic unroll.
At least it needs no-runtime. Will try to keep it to the minimal set.

This revision was automatically updated to reflect the committed changes.
fedor.sergeev marked an inline comment as done.Oct 31 2018, 9:24 AM