This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Separate Peeling Properties into its own struct
ClosedPublic

Authored by sidbav on May 26 2020, 12:42 PM.

Details

Summary

Currently, all of the Loop Peeling Properties are found in the TargetTransformInfo::UnrollingPreferences struct (llvm/include/llvm/Analysis/TargetTransformInfo.h).

Therefore, other loop transforms (i.e., LoopFusion) cannot access the peeling properties of a loop as it right now.

Diff Detail

Event Timeline

sidbav created this revision.May 26 2020, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 12:42 PM
sidbav edited the summary of this revision. (Show Details)May 26 2020, 12:44 PM
fhahn added a subscriber: fhahn.May 26 2020, 1:09 PM

Could you elaborate why this change is desired? Is there a problem with getting the unrolling preferences and checking the peeling properties in that way? If that's the case it would be good to provide some context (e.g. in form of a follow-on patch that uses the new PeelingPreferences).

sidbav edited the summary of this revision. (Show Details)May 26 2020, 1:24 PM

Could you elaborate why this change is desired? Is there a problem with getting the unrolling preferences and checking the peeling properties in that way? If that's the case it would be good to provide some context (e.g. in form of a follow-on patch that uses the new PeelingPreferences).

I would not really call it a problem, but more of an inconvenience. Peeling should not be attached to Unrolling if other Transforms can also make use of peeling. Additionally when another transformation is accessing the peeling properties, I do not think it would make sense to do so through the UnrollingPreferences struct. This could make for some quite confusing code.

However, I can start to look into a follow-up patch which makes use of the PeelingPreferences struct.

fhahn added a comment.May 26 2020, 1:47 PM

Could you elaborate why this change is desired? Is there a problem with getting the unrolling preferences and checking the peeling properties in that way? If that's the case it would be good to provide some context (e.g. in form of a follow-on patch that uses the new PeelingPreferences).

I would not really call it a problem, but more of an inconvenience. Peeling should not be attached to Unrolling if other Transforms can also make use of peeling. Additionally when another transformation is accessing the peeling properties, I do not think it would make sense to do so through the UnrollingPreferences struct. This could make for some quite confusing code.

However, I can start to look into a follow-up patch which makes use of the PeelingPreferences struct.

Ok great, it would be good to show a clear benefit, as this change has the potential to add some churn for out-of-tree maintainers/targets, which is fine, if there's a clear benefit.

sidbav added a comment.EditedMay 26 2020, 2:17 PM

@fhahn I think a follow-up patch will take a little bit of time to develop so here is a quick example.

for (i = 0; i < 10; ++i)
  a[i] = a[i] + 3;
for (j = 1; j < 10; ++j)
  b[j] = b[j] + 5;

Here is we can make use of peeling, and then fuse the two loops together. We can peel off the 0th iteration of the loop i, and then combine loop i and j for i = 1 to 10.

a[0] = a[0] +3;
for (i = 1; i < 10; ++i) {
  a[i] = a[i] + 3;
  b[i] = b[i] + 5;
}
sidbav updated this revision to Diff 274663.Jun 30 2020, 6:29 PM

Some NFC modifications.

@fhahn added the followup patch D82927. Also ping at reviewers of this patch! Thanks!

Added @hfinkel as reviewer who introduced UnrollingPreferences.

Unfortunatey, UnrollingPreferences mixes unroll/peel decisions for a specific loop with settings on how/whether to unroll/peel loops in general.

Given this preexisting issue, the patch itself look fine.

Meinersbur accepted this revision.Jul 7 2020, 10:14 AM

Given there is no other feedback, I accept the patch.

This revision is now accepted and ready to land.Jul 7 2020, 10:14 AM
sidbav retitled this revision from Separate Peeling Properties into its own struct to [NFC] Separate Peeling Properties into its own struct.Jul 7 2020, 10:44 AM
This revision was automatically updated to reflect the committed changes.
sidbav updated this revision to Diff 277025.Jul 10 2020, 7:12 AM
sidbav added reviewers: anhtuyen, nikic.

Applying the patch in its previous state resulted in build failures. Updated patch to resolve all build failures. Major change is in the file llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp in the gatherPeelingPreferences function.

Patch still look fine to me.