Page MenuHomePhabricator

Separate the Loop Peeling Utilities from the Loop Unrolling Utilities
ClosedPublic

Authored by sidbav on Jul 2 2020, 8:33 AM.

Details

Summary

This patch separates the loop peeling utilities from Loop Unrolling to generalize it since it is no longer just being used by Loop Unrolling. (D82927).

This patch is a followup to D80580. In that patch I separate the peeling specific parameters in the UnrollingPreferences struct and form a new struct called PeelingPreferences.

This patch is also a followup to D82927, where loop peeling is added to loop fusion.

The diff of this patch was created assuming that D80580 and D82927 is merged.

Diff Detail

Event Timeline

sidbav created this revision.Jul 2 2020, 8:33 AM

Please put NFC in the title if no functional changes are intended. The same comment applies to D80580. Thanks.

fhahn added inline comments.Jul 7 2020, 10:24 AM
llvm/lib/Transforms/Utils/LoopPeel.cpp
50

I am not sure about this change. Currently peeling is integrated in loop-unroll and remarks/debug can be filtered by loop-unroll, but now we will generate remarks for loop-unroll and loop-peel when running -loop-unroll.

sidbav retitled this revision from Separate the Loop Peeling Utilities from the Loop Unrolling Utilities to [NFC] Separate the Loop Peeling Utilities from the Loop Unrolling Utilities .Jul 7 2020, 10:44 AM
Meinersbur added inline comments.Jul 7 2020, 12:10 PM
llvm/lib/Transforms/Utils/LoopPeel.cpp
50

Isn't it actually better since you can now filter -debug-only=loop-unroll, respectively -debug-only=loop-peel depending on what you want to look at?

Note: -Rpass= remarks use the pass name, not DEBUG_TYPE.

sidbav marked an inline comment as done.Jul 7 2020, 2:49 PM
sidbav added inline comments.
llvm/lib/Transforms/Utils/LoopPeel.cpp
50

I also agree with @Meinersbur, having them separate is better. Additionally, in the case that the developer wants to look at both unrolling and peeling at the same time, they can specify debug-only=loop-unroll,loop-peel.

fhahn added inline comments.Jul 7 2020, 3:05 PM
llvm/lib/Transforms/Utils/LoopPeel.cpp
50

Isn't it actually better since you can now filter -debug-only=loop-unroll, respectively -debug-only=loop-peel depending on what you want to look at?

I'd say it depends. Personally I find it mostly makes things less discoverable for newcomers. I can see how it might be surprising if a user wants to ask for debug output of the LoopUnroll pass and then the pass makes changes but doesn't display the debug output. It's certainly not a new problem though and not a blocker. I think it means that the patch changes behavior though ;)

Meinersbur added inline comments.Jul 7 2020, 4:58 PM
llvm/lib/Transforms/Utils/LoopPeel.cpp
50

If you want both, use -debug-only=loop-unroll,loop-peel. For discoverability one can emit everything using just -debug. Ultimately, every change can be surprising.

DEBUG_TYPE is for debugging, not an intentional behavioral change (in debug builds). I agree this is a somewhat grey area for a NFC(I) patch, but in the strict interpretation assertion also may change behavior and we change those regularly in refactoring NFC changes. I don't think we have a policy for this case.

sidbav updated this revision to Diff 277145.Jul 10 2020, 2:17 PM
sidbav marked an inline comment as not done.

Minor update after making some modifications to D80580

Ping @ reviewers! Thanks!

fhahn added inline comments.Fri, Jul 17, 7:02 AM
llvm/lib/Transforms/Utils/LoopPeel.cpp
16

It seems like some of the new includes are not being used? Are they all required?

36

I think stuff in Debug.h/CommandLine.h is directly used in this file (e.g. cl::opt is defined in CommandLine.h I think). Why are those removed? (I did not check all other includes, but suspect there are more that should not be removed)

50

If you want both, use -debug-only=loop-unroll,loop-peel. For discoverability one can emit everything using just -debug. Ultimately, every change can be surprising.

Yep, I can't think of a better alternative either.

DEBUG_TYPE is for debugging, not an intentional behavioral change (in debug builds). I agree this is a somewhat grey area for a NFC(I) patch, but in the strict interpretation assertion also may change behavior and we change those regularly in refactoring NFC changes. I don't think we have a policy for this case.

Sure, and the distinction is not very important IMO. But technically a new assertion should never trigger (and nothing is observable), but changing DEBUG_TYPE changes observable behavior in debug builds.

626

nit: usually 'full' sentences are used for comments, so it might be good to add a period at the end and not capitalize words in the middle of the sentence, both there and in the other comments.

sidbav updated this revision to Diff 278869.Fri, Jul 17, 11:34 AM

Address Florian 's comments.

sidbav marked 3 inline comments as done.Fri, Jul 17, 11:34 AM
Meinersbur added inline comments.Sun, Jul 19, 8:14 PM
llvm/lib/Transforms/Utils/LoopPeel.cpp
50

shouldReturnTrue() in assert(shouldReturnTrue()) might print to dbgs() and increase statistics counters and therefore also changes observable behavior.

We could just remove the [NFC] tag from the title, it's a gray area anyway.

sidbav retitled this revision from [NFC] Separate the Loop Peeling Utilities from the Loop Unrolling Utilities to Separate the Loop Peeling Utilities from the Loop Unrolling Utilities .Mon, Jul 20, 7:08 AM
sidbav marked 7 inline comments as done.

@fhahn Any remaining concerns? I would accept the patch if not.

fhahn added a comment.Wed, Jul 22, 5:57 AM

@fhahn Any remaining concerns? I would accept the patch if not.

I think everything besides the remaining question about the include changes has been addressed.

llvm/lib/Transforms/Utils/LoopPeel.cpp
28

I am still not sure why those includes were removed? If they are not needed independently of this change, it should be a separate patch. Usually we include what is used in each .cpp file. Things might build because some headers are included by other headers in turn, but usually we do not rely on that.

sidbav updated this revision to Diff 280140.Thu, Jul 23, 8:28 AM

Include all of the includes from before, and update patch based on changes in D82927

sidbav marked 2 inline comments as done.Thu, Jul 23, 8:29 AM
sidbav added inline comments.
llvm/lib/Transforms/Utils/LoopPeel.cpp
28

Updated the file to such that all of the includes from before are included.

sidbav marked an inline comment as done.Thu, Jul 30, 8:15 AM

Ping!

Meinersbur accepted this revision.Thu, Jul 30, 8:32 AM
This revision is now accepted and ready to land.Thu, Jul 30, 8:32 AM

Please put NFC in the title if no functional changes are intended. The same comment applies to D80580. Thanks.

sidbav added a comment.Sat, Aug 1, 6:08 AM

Hey @sckmodifier, please take a look at some comments after that one you just quoted. I added the NFC tag, but then in later discussion, it was determined it is best to remove the NFC tag since this change is in somewhat in a grey area.