Page MenuHomePhabricator

[WIP][NFC] factor out unrolling decision logic
Needs ReviewPublic

Authored by Ali-Sed on Jun 30 2021, 12:04 AM.

Details

Reviewers
mtrofin
jdoerfert
Summary

moving runtime flag to the Logic

minor bug

[WIP] minor bug

[WIP] changes by clang-format

[WIP] merge conflicts

[WIP][UNROLLING] merge conflicts

Merge branch 'main' of github.com:Ali-Sed/llvm-project into main

Diff Detail

Event Timeline

Ali-Sed created this revision.Jun 30 2021, 12:04 AM
Ali-Sed requested review of this revision.Jun 30 2021, 12:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2021, 12:04 AM
Ali-Sed updated this revision to Diff 356193.Jul 2 2021, 9:31 AM

[WIP] Reformatting using clang-reformat

Ali-Sed updated this revision to Diff 356202.Jul 2 2021, 9:59 AM

Squashing all commits

Could you update the commit message (both git and here) to say what the patch accomplishes, i.e. "[NFC] factor out unrolling decision logic" or something like that - nfc because it should just be a refactoring. The message can clarify why - e.g. so it may simplify exploring alternative decision-making mechanisms.

llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
761

use verb names for functions, e.g. shouldUnroll. That also explains what the return means.

Also add a comment above it, like computeUnrollCount has.

does MaxTripCount need to be passed by-ref?

could the return of this function be rather a more complex object, where the TripCount and TripMultiple were returned - I haven't looked yet at how it's used, is it that the caller proposes such values, and this adjusts them?

772

nit: if UserUnrollCount doesn't change later, just const - it. The reader then understands this is meant to help with understanding the code. Same below.

957–958

why remove the comment?

958–960

capital 'T'.

And/but, if you want to do this for clarity wrt what the params mean, 3 options:

  1. no variable name. Instead (the way commonly seen in the codebase):

unrollingLogic(/*TryFullUnroll=*/true, ...)

  1. just do this (but it's not common in this codebase)

const bool DoFullUnroll = true;
const bool DoPartialUnroll = false;

  1. define an enum class:

enum class UnrollingOption : bool {

Full,
Partial

};

...you see where this is going

974

isn't clear where the 1-3 priorities are, just add a comment about that

987

it seems like the only difference between the first call and this is whether to try full unroll. We could, instead, return an enum from <what's currently called unrollingLogic>.

*or* factor that function in 2: shouldFullyUnroll, shouldPartiallyUnrol

1026

don't comment code, remove it instead (if you don't need it)

Ali-Sed updated this revision to Diff 356586.Jul 5 2021, 6:45 PM

Adding NFC tag

Ali-Sed retitled this revision from Initial refactoring to [WIP][NFC] factor out unrolling decision logic.Jul 5 2021, 6:47 PM
Ali-Sed updated this revision to Diff 356878.EditedJul 6 2021, 11:53 PM

Solving the problems mentioned in the review

dmgreen added a subscriber: dmgreen.Jul 7 2021, 3:08 AM

The latest revision appears to be missing most of the changes. You may want to squash several commits together in git, and make sure the commit message properly explains the reasoning behind the patch.

Do you intend this to go into LLVM, or are you still experimenting at this point? It may be best to make sure what you come up with will have practical use before making a lot of changes in llvm itself. My understanding is that this code is (overly) complex but it is the order it is for deliberate reasons. All the existing tests have to keep passing, if they are not already.

I would consider runtime unrolling to be the most interesting part of machine learning loop unroll factors, at least on many cpu's.

Ali-Sed updated this revision to Diff 358392.Jul 13 2021, 12:44 PM

[WIP][NFC] Adding enum for unrolling option and adding MyDebug for priting

Ali-Sed added a comment.EditedJul 13 2021, 12:53 PM

The latest revision appears to be missing most of the changes. You may want to squash several commits together in git, and make sure the commit message properly explains the reasoning behind the patch.

Do you intend this to go into LLVM, or are you still experimenting at this point? It may be best to make sure what you come up with will have practical use before making a lot of changes in llvm itself. My understanding is that this code is (overly) complex but it is the order it is for deliberate reasons. All the existing tests have to keep passing, if they are not already.

I would consider runtime unrolling to be the most interesting part of machine learning loop unroll factors, at least on many cpu's.

Thanks a lot for your feedback.
I've just squashed all of the commits now and used "arc diff" for a group of commits and it seems it solved the problem.

I've been decoupling the logic, that unrolling pass uses, from the rest of it. So in a near future, I'll be able to replace it with a learning model.

For now, full unrolling and partial unrolling are the main goals. But, yes definitely! the logic behind the runtime unrolling could be a very interesting choice to be replaced with a learning model too.

Ali-Sed updated this revision to Diff 358458.Jul 13 2021, 4:50 PM

"Adding ShouldContinue as the return value of unrolling logic"