This is an archive of the discontinued LLVM Phabricator instance.

[LoopNest][LoopFlatten] Change LoopFlattenPass to LoopNest pass
ClosedPublic

Authored by eopXD on May 21 2021, 12:53 AM.

Details

Diff Detail

Event Timeline

eopXD created this revision.May 21 2021, 12:53 AM
eopXD requested review of this revision.May 21 2021, 12:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2021, 12:53 AM
eopXD updated this revision to Diff 346949.May 21 2021, 1:24 AM

Apply clang-format.

eopXD updated this revision to Diff 346952.May 21 2021, 1:35 AM

Utilize LoopNest in Flatten, this shall eliminate repeating calls to FlattenLoopPair

eopXD retitled this revision from [LoopNest] Change LoopFlattenPass to LoopNest pass to [LoopNest][LoopFlatten] Change LoopFlattenPass to LoopNest pass.May 21 2021, 2:02 AM
eopXD updated this revision to Diff 346965.May 21 2021, 2:41 AM

Apply clang-tidy.

Herald added a reviewer: shafik. · View Herald Transcript
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a reviewer: rriddle. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: int3. · View Herald Transcript
Herald added a reviewer: sjarus. · View Herald Transcript
Herald added a reviewer: gkm. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
eopXD removed projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project.
eopXD removed subscribers: JDevlieghere, arsenm, dschuff and 89 others.
Herald added a reviewer: sjarus. · View Herald Transcript
Herald added a reviewer: gkm. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
eopXD updated this revision to Diff 346967.May 21 2021, 2:50 AM

Reset update on files.

eopXD edited the summary of this revision. (Show Details)May 21 2021, 2:50 AM
eopXD removed reviewers: andreadb, alexander-shaposhnikov, shafik, rupprecht, jdoerfert, jhenderson, sstefan1, nicolasvasilache, herhut, rriddle, sscalpone, ftynse, aaron.ballman, baziotis, aartbik, int3, sjarus, gkm, Restricted Project, Restricted Project, Restricted Project.
eopXD removed projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project.
eopXD edited the summary of this revision. (Show Details)
Whitney added inline comments.May 21 2021, 1:39 PM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
71

Please avoid unnecessary changes.

738–739

We can keep the LPM manager as FunctionPass, as after this change, a loop nest can be transform repeatedly the number of loops time.

eopXD updated this revision to Diff 347180.May 22 2021, 12:48 AM

address comments.

eopXD marked 2 inline comments as done.May 22 2021, 12:50 AM
Whitney added inline comments.May 23 2021, 6:28 AM
llvm/lib/Passes/PassRegistry.def
425

Please undo this no newline change.

llvm/lib/Transforms/Scalar/LoopFlatten.cpp
92

Please avoid unnecessary changes.
Same goes to other changes.

eopXD updated this revision to Diff 347311.May 23 2021, 10:41 PM

revert the changes caused by clang-format.

eopXD updated this revision to Diff 347312.May 23 2021, 10:44 PM

more recoveries from clang-format.

eopXD updated this revision to Diff 347313.May 23 2021, 10:53 PM

Added new lines at end of file,
also removed the redundant header I added.

D99149 ended up needing to use the LPMUpdater as the loops may be removed. Does the same thing need to be done here? As loop flattening does inherently remove loops.

eopXD added a comment.EditedMay 24 2021, 1:53 AM

D99149 ended up needing to use the LPMUpdater as the loops may be removed. Does the same thing need to be done here? As loop flattening does inherently remove loops.

Loop flattening aims to remove the inner loop of a nested loop,
but under LoopNestMode there is an assertion in LPMUpdater::markLoopAsDeleted that checks if the deleting loop the outer-most loop.
I think the assertion should not hold, the function shall allow markLoopAsDeleted for sub-loop in LoopNestMode,
and check that whether the mark-deleted-loop is a sub-loop, then call revisitCurrentLoop if so.

Thank you for spotting out the problem.

eopXD updated this revision to Diff 347340.May 24 2021, 3:06 AM

added LPMUpdater for loop removal,
updated LPMUpdater::markLoopAsDeleted, allow sub-loop to be deleted in LoopNestMode.

Whitney added inline comments.May 24 2021, 5:52 AM
llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
262 ↗(On Diff #347340)

Can you please explain why should revisitCurrentLoop?

llvm/lib/Transforms/Scalar/LoopFlatten.cpp
742

There can be more than one loop nest in a function. You may need a loop like for (auto *InnerLoop : LI->getLoopsInPreorder()) { here.

eopXD added a subscriber: dmgreen.May 24 2021, 7:33 AM

@Whitney : Can you please explain why should revisitCurrentLoop?

I think I have misused revisitCurrentLoop here, the loop don't need to be revisited again,
because Flatten will be called for go through the possible inner/outer loop pairs.

@dmgreen
In LoopNestMode, sub-loops are not in the work list of LPMUpdater, so inner-loops can't be marked by deleted.
I think wrong for removing the assertion, since LAM.clear(L, Name); won't find the sub-loop, and markLoopAsDelete shall not be called for the inner-loop.
May you correct me if I am wrong.

eopXD updated this revision to Diff 347390.May 24 2021, 7:37 AM
  1. revert change for LPMUpdater::markLoopAsDeleted, remove the call also.
  2. iterate through Loops and record top-level loops for legacy LoopFlattenPass, a seperate for-loop is called because LoopFlatten may affect the range.
eopXD updated this revision to Diff 347393.May 24 2021, 7:43 AM

remove redundant LPMUpdater parameter.

eopXD marked 3 inline comments as done.May 24 2021, 7:45 AM

OK. So because we only delete the inner loop, not the current one, the loop nest pass remains valid? That sounds Ok then.

I think the changes from UnrollAndJam were found with a sanitizer build, so it might be worth testing it with one of those (or letting the buildbots do it post-commit, and keeping an eye one them to make sure they remain OK).

llvm/lib/Transforms/Scalar/LoopFlatten.cpp
682

We can remove the LPMUpdater again if it's not needed/used?

748

I think this can just be for (Loop *L : *LI), then it will only include the outermost loops.

eopXD updated this revision to Diff 347566.May 24 2021, 8:53 PM

address comments.

eopXD marked 2 inline comments as done.EditedMay 24 2021, 9:00 PM

We can remove the LPMUpdater again if it's not needed/used?

I think for a LoopNestPass (or a LoopPass), the LPMUpdater is necessary parameter.
As LoopNestPass is added by createFunctionToLoopPassAdaptor< LoopPassManager >(),
it needs to takes LPMUpdater & even though it is not used.

Whitney added inline comments.May 25 2021, 7:11 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
733

Why do we need these 3 new dependencies?

eopXD added a comment.May 25 2021, 7:54 AM

@Whitney : do we need these 3 new dependencies?

I've deleted the parts for loop simplification and form LCSSA inside function Flatten,
and thought this would lead to the legacy pass depending on the passes I added.

I've just commented out the 3 passes and the tests still passed when I added -enable-new-pm=0 (using the legacy pass).
I think this means the 3 dependencies are unnecessary then, I will remove my addition.

eopXD updated this revision to Diff 347687.EditedMay 25 2021, 8:33 AM

remove redundant pass dependencies added.

eopXD marked an inline comment as done.May 25 2021, 8:33 AM
eopXD updated this revision to Diff 347688.May 25 2021, 8:35 AM

remove redundant header added.

Whitney accepted this revision.May 25 2021, 9:16 AM

LGTM.

This revision is now accepted and ready to land.May 25 2021, 9:16 AM
This revision was landed with ongoing or failed builds.May 28 2021, 12:11 AM
This revision was automatically updated to reflect the committed changes.
eopXD reopened this revision.May 28 2021, 12:25 AM
This revision is now accepted and ready to land.May 28 2021, 12:25 AM
eopXD updated this revision to Diff 348456.May 28 2021, 12:25 AM

Reverting my commit to investigate for build failure.

This revision was landed with ongoing or failed builds.May 28 2021, 12:26 AM
This revision was automatically updated to reflect the committed changes.
eopXD reopened this revision.May 28 2021, 12:27 AM
This revision is now accepted and ready to land.May 28 2021, 12:27 AM
eopXD updated this revision to Diff 348526.May 28 2021, 7:43 AM

Resolve merge conflict correctly. (previously caused build fail)

eopXD updated this revision to Diff 348532.May 28 2021, 8:14 AM

Rebase code to main.

eopXD updated this revision to Diff 348536.May 28 2021, 8:33 AM

Remove declaration of LoopUnrollAndJam as a FunctionPass.

Previously I resolved the merge conflict incorrecly.
LoopUnrollAndJam is now a LoopPass.

This revision was landed with ongoing or failed builds.May 28 2021, 8:43 AM
This revision was automatically updated to reflect the committed changes.