This is an archive of the discontinued LLVM Phabricator instance.

[LoopFlatten] Make it a FunctionPass
ClosedPublic

Authored by SjoerdMeijer on Nov 6 2020, 6:47 AM.

Details

Summary

This converts LoopFlatten from a LoopPass to a FunctionPass so that we don't run into problems of a loop pass deleting a (inner)loop.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Nov 6 2020, 6:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2020, 6:47 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
SjoerdMeijer requested review of this revision.Nov 6 2020, 6:47 AM
dmgreen added inline comments.Nov 6 2020, 7:08 AM
llvm/include/llvm/Transforms/Scalar.h
340

Is there an important ordering to this file? It's not alphabetical..

Any reason why it's moved?

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

Should this be handling sub-loops too? Maybe use getLoopsInPreorder if that works how I think it does?

574

Does this preserve all the things a loop pass would? I think the answer is yes, but it's perhaps worth checking. LI seems to be kept up to date, which was the one I was worried about.

SjoerdMeijer added inline comments.Nov 6 2020, 7:24 AM
llvm/include/llvm/Transforms/Scalar.h
340

Thanks for looking at this Dave.

I thought of clustering it with the other FunctionPasses, but now I see that's not really true (it's mixed already), so will move it back.

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

Ah, I guess so. This will flatten only doubly nested loops, but won't flatten j and k in a 3d loop:

for i
  for j
    for k

Looks like a test is missing too...

574

Will double check.

SjoerdMeijer added inline comments.Nov 6 2020, 7:26 AM
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
536

But maybe this is not supported anyway, will check.

Comments addressed, and have added some test cases for 3 deep loop nests.

dmgreen added inline comments.Nov 9 2020, 12:04 PM
llvm/test/Transforms/LoopFlatten/loop-flatten-negative.ll
404

None of these are flattened? Any idea why?

You can remove dso_local and local_unnamed_addr #0

SjoerdMeijer added inline comments.Nov 9 2020, 12:24 PM
llvm/test/Transforms/LoopFlatten/loop-flatten-negative.ll
404

None of these are flattened? Any idea why?

Yeah, looks like there's room for improvement. They are rejected for different reasons:

  • Did not match expected pattern, bailing
  • Cannot flatten because instruction may have side effects
  • Might overflow, to be addressed in a next patch,

Addressed comments: cleaned up the new test cases a bit.

dmgreen accepted this revision.Nov 10 2020, 10:49 AM

Thanks for checking. LGTM

This revision is now accepted and ready to land.Nov 10 2020, 10:49 AM
This revision was automatically updated to reflect the committed changes.
yrouban added inline comments.
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
558

CFG is changed. Does this pass update CFGAnalyses on its own? If not, I believe this line should be removed.