Page MenuHomePhabricator

[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

Unit TestsFailed

TimeTest
90 mswindows > LLVM.Analysis/ScalarEvolution::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\w2\llvm-project\premerge-checks\build\bin\opt.exe < C:\ws\w2\llvm-project\premerge-checks\llvm\test\Analysis\ScalarEvolution\2012-05-29-MulAddRec.ll -S -indvars -loop-unroll | c:\ws\w2\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w2\llvm-project\premerge-checks\llvm\test\Analysis\ScalarEvolution\2012-05-29-MulAddRec.ll
200 mswindows > LLVM.Analysis/ScalarEvolution::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\w2\llvm-project\premerge-checks\build\bin\opt.exe < C:\ws\w2\llvm-project\premerge-checks\llvm\test\Analysis\ScalarEvolution\scev-expander-reuse-unroll.ll -loop-unroll -unroll-runtime -unroll-count=2 -verify-scev-maps -S | c:\ws\w2\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w2\llvm-project\premerge-checks\llvm\test\Analysis\ScalarEvolution\scev-expander-reuse-unroll.ll
120 mswindows > LLVM.CodeGen/Thumb2::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\w2\llvm-project\premerge-checks\build\bin\opt.exe < C:\ws\w2\llvm-project\premerge-checks\llvm\test\CodeGen\Thumb2\2009-12-01-LoopIVUsers.ll -O3 | c:\ws\w2\llvm-project\premerge-checks\build\bin\llc.exe -mtriple=thumbv7-apple-darwin10 -mattr=+neon | c:\ws\w2\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w2\llvm-project\premerge-checks\llvm\test\CodeGen\Thumb2\2009-12-01-LoopIVUsers.ll
90 mswindows > LLVM.Other::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\w2\llvm-project\premerge-checks\build\bin\opt.exe -S -O3 < C:\ws\w2\llvm-project\premerge-checks\llvm\test\Other\cleanup-lcssa.ll | c:\ws\w2\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w2\llvm-project\premerge-checks\llvm\test\Other\cleanup-lcssa.ll
170 mswindows > LLVM.Other::Unknown Unit Message ("")
Script: -- : 'RUN: at line 4'; c:\ws\w2\llvm-project\premerge-checks\build\bin\opt.exe < C:\ws\w2\llvm-project\premerge-checks\llvm\test\Other\loop-pass-printer.ll 2>&1 -disable-output -loop-deletion -print-before=loop-deletion | c:\ws\w2\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w2\llvm-project\premerge-checks\llvm\test\Other\loop-pass-printer.ll -check-prefix=DEL
View Full Test Results (74 Failed)

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.Tue, Jul 7, 10:14 AM

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

This revision is now accepted and ready to land.Tue, Jul 7, 10:14 AM
sidbav retitled this revision from Separate Peeling Properties into its own struct to [NFC] Separate Peeling Properties into its own struct.Tue, Jul 7, 10:44 AM
This revision was automatically updated to reflect the committed changes.
sidbav updated this revision to Diff 277025.Fri, Jul 10, 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.